wip showcase options tracing tests #29709

maorleger · 2024-05-15T19:54:49Z

Examining various options to fix tracing tests, as we discussed.

Plumber work to get the correct instrumenter regardless of core-tracing
versions fe29268
Mock createTracingClient and assert on mock 3e6a540

Should be reviewed commit by commit

maorleger · 2024-05-15T19:55:10Z

sdk/appconfiguration/app-configuration/package.json

@@ -41,7 +41,7 @@
    "audit": "node ../../../common/scripts/rush-audit.js && rimraf node_modules package-lock.json && npm i --package-lock-only 2>&1 && npm audit",
    "build:test": "tsc -p . && dev-tool run bundle",
    "build:samples": "echo Obsolete.",
-    "build": "npm run clean && tsc -p . && dev-tool run bundle && dev-tool run extract-api",
+    "build": "npm run clean && tsc -p . &&  dev-tool run extract-api",


ignore, just speeding things up

maorleger · 2024-05-15T19:55:37Z

sdk/appconfiguration/app-configuration/package.json

@@ -89,7 +89,7 @@
    "@azure/core-lro": "^2.5.1",
    "@azure/core-paging": "^1.4.0",
    "@azure/core-rest-pipeline": "^1.6.0",
-    "@azure/core-tracing": "^1.0.0",
+    "@azure/core-tracing": "1.1.3",


pin to 1.1.3, while test-utils pins to 1.1.2 - this allows me to update test-utils without patching node_modules

maorleger · 2024-05-15T19:56:08Z

sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts

+      namespace: "Microsoft.AppConfiguration",
+      packageName: "@azure/app-configuration",
+      packageVersion,
+      instrumenter: appConfigOptions.instrumenter,


Moved to client constructor so we can pass it through

maorleger · 2024-05-15T19:56:28Z

sdk/appconfiguration/app-configuration/test/public/tracing.spec.ts

  let recorder: Recorder;

  beforeEach(async function (this: Context) {
    recorder = await startRecorder(this);
    client = createAppConfigurationClientForTests(recorder.configureClientOptions({}));
+    fixedClient = createAppConfigurationClientForTests(
+      recorder.configureClientOptions({
+        instrumenter,


this fixedClient plumbs the thing through

maorleger · 2024-05-15T19:57:04Z

sdk/appconfiguration/app-configuration/test/public/tracing.spec.ts

  });

  afterEach(async () => {
    await recorder.stop();
  });

  it("can trace through the various options", async function () {
+    console.log("expected to fail with missing spans");


To avoid dealing with recorder, I just name the tests with the exact same name, but the console log should match and we should see two pass and one fail

azure-sdk · 2024-05-15T20:05:37Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

@azure/core-tracing

maorleger · 2024-05-15T20:53:35Z

sdk/appconfiguration/app-configuration/test/public/tracing.spec.ts

+    );
+  });
+
+  it("can trace through the various options", async function () {


This is the other option, which we can centralize of course and clean up, but the TL;DR; is

mock in some way that you can check the span names

check that withSpan was called N times

check that the span names match

I like the other option better

To me the mock approach is more of a true unit test, but if we're really trying to validate scenario correctness, the extra plumbing of passing the instrumenter seems fairly low overhead and feels good from a dependency injection POV.

Yeah the more I look at it the more I am on the fence 😄 I like that we don't have to add anything to the public API to support these tests and I think the test coverage is sufficient - core-rest-pipeline which outputs the most important spans (the HTTP spans) doesn't use this helper today so that remains unchanged.

I think out of the two, what will help me is to sync up with @mpodwysocki and get a better understanding on which approach will play nicer with vitest and ESM. That will likely help sway one way or another...

If we decided to go this approach though, I can own updating all the packages and tests - it'll be tedious but not too hard...

I'm with @xirzec that we should test for correctness and passing through with dependency injection seems a bit nicer than replacing globals.

SGTM, I created this issue to track that work #29736 as a longer term fix

mpodwysocki · 2024-05-16T20:54:31Z

sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts

@@ -73,14 +72,17 @@ import {
  transformKeyValueResponseWithStatusCode,
  transformSnapshotResponse,
 } from "./internal/helpers";
+import { SyncTokens, syncTokenPolicy } from "./internal/synctokenpolicy";
+import { TokenCredential, isTokenCredential } from "@azure/core-auth";


Nit to use type when possible for imports.

mpodwysocki · 2024-05-17T15:10:41Z

sdk/core/core-tracing/src/tracingClient.ts

@@ -2,6 +2,7 @@
 // Licensed under the MIT license.

 import {
+  Instrumenter,


Nit once again to import type

### Packages impacted by this PR @azure-tools/test-utils ### Issues associated with this PR Resolves #29736 ### Describe the problem that is addressed by this PR When @azure-tools/test-utils moves to `npm` it will no longer default to the on-disk version of core-tracing. This impacts our `supportsTracing` test helper as it no longer sets the instrumenter on the same version as the client package. Imagine this flow: <img src="https://github.com/Azure/azure-sdk-for-js/assets/753570/12d52e8e-16c1-44b3-b57d-edd31a1e4627" width=70% height=70%> In this case, test-util and app-configuration are not talking to the same "module global" instrumenter. This PR fixes this by removing core-tracing from test-utils, setting it as a peerDependency instead. All packages that test tracing also implement tracing, and as such are already depending on core-tracing. This was verified locally, but I'll need to get it out to NPM before verification can complete ### Alternatives See #29709 for a few alternatives: 1. Pass the instrumenter through 2. Use mocks Both require updates to multiple packages and/or adding public API surface which is unnecessary. We can fallback to those options if we have to, but this is a promising alternative.

github-actions bot added App Configuration Azure.ApplicationModel.Configuration Azure.Core test-utils Label for the issues related to the @azure/test-utils package labels May 15, 2024

maorleger commented May 15, 2024

View reviewed changes

maorleger requested review from xirzec and mpodwysocki May 15, 2024 19:57

maorleger commented May 15, 2024

View reviewed changes

Plumbing work

fe29268

maorleger force-pushed the wip-showcase-options-tracing-tests branch 2 times, most recently from 76d04e6 to a6e4e49 Compare May 15, 2024 22:13

Add some basic stubbing

3e6a540

maorleger force-pushed the wip-showcase-options-tracing-tests branch from a6e4e49 to 3e6a540 Compare May 15, 2024 22:21

mpodwysocki reviewed May 16, 2024

View reviewed changes

mpodwysocki reviewed May 17, 2024

View reviewed changes

maorleger closed this May 17, 2024

maorleger mentioned this pull request May 21, 2024

[test-util] Move core-tracing to peerDependencies #29775

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wip showcase options tracing tests #29709

wip showcase options tracing tests #29709

maorleger commented May 15, 2024 •

edited

maorleger May 15, 2024

maorleger May 15, 2024

maorleger May 15, 2024

maorleger May 15, 2024

maorleger May 15, 2024 •

edited

azure-sdk commented May 15, 2024 •

edited

maorleger May 15, 2024

xirzec May 15, 2024

maorleger May 15, 2024

mpodwysocki May 17, 2024

maorleger May 17, 2024

mpodwysocki May 16, 2024

mpodwysocki May 17, 2024

wip showcase options tracing tests #29709

wip showcase options tracing tests #29709

Conversation

maorleger commented May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maorleger May 15, 2024 • edited

Choose a reason for hiding this comment

azure-sdk commented May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maorleger commented May 15, 2024 •

edited

maorleger May 15, 2024 •

edited

azure-sdk commented May 15, 2024 •

edited