Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracing: Unable to use multiple versions of core-tracing #29736

Open
maorleger opened this issue May 16, 2024 · 1 comment · Fixed by #29775
Open

Tracing: Unable to use multiple versions of core-tracing #29736

maorleger opened this issue May 16, 2024 · 1 comment · Fixed by #29775
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. test-utils Label for the issues related to the @azure/test-utils package
Milestone

Comments

@maorleger
Copy link
Member

maorleger commented May 16, 2024

Now that we have multiple versions of core-tracing being used side-by-side the supportsTracing helper has been failing - test-utils will set the tracer on version N, but the client package uses the tracer on version N+1

To resolve this, we want to:

  • Add Instrumenter to tracingOptions in core-tracing
  • Pass the mockInstrumenter instance from test-utils through the tracing client
  • Use that instance when it is provided

Then, each client can instantiate its own tracingClient passing the instrumenter through and allowing us to ensure the same instance is shared across versions.

It would be nice if we can hook this into the recorder.configureClientOptions call to avoid having to patch the instrumenter in every test file

@maorleger maorleger added Client This issue points to a problem in the data-plane of the library. test-utils Label for the issues related to the @azure/test-utils package labels May 16, 2024
@maorleger maorleger added this to the 2024-06 milestone May 16, 2024
@maorleger maorleger self-assigned this May 16, 2024
@maorleger maorleger changed the title Tracing: Migrate away from supportsTracing Tracing: Support passing a custom instrumenter May 17, 2024
@maorleger maorleger modified the milestones: 2024-06, 2024-07 May 17, 2024
maorleger added a commit that referenced this issue May 23, 2024
### 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.
@maorleger maorleger reopened this May 28, 2024
@maorleger maorleger changed the title Tracing: Support passing a custom instrumenter Tracing: Unable to use multiple versions of core-tracing May 29, 2024
@maorleger
Copy link
Member Author

As a short-term workaround, we've agreed to pin to the latest released version of @azure/core-tracing in order to consolidate all versions to one in our repo. This is a workaround, not a full-blown solution; however, if we can get everyone on one version of @azure-tools/test-utils (and that version is the in-repo version) we will be able to remove that workaround

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. test-utils Label for the issues related to the @azure/test-utils package
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant