Fix core-tracing duplication #5389
Conversation
xirzec
added
Client
xirzec
force-pushed
the
fixCoreTracingDuplication
branch
from
7eaf255 to
44f5675
Compare
|
@chradek I fixed the issue you were talking about before with having two copies of core-tracing. It was caused by a rollup configuration issue. With the fix from this PR I was able to locally verify that TracingPolicy spans show up correctly for the storage packages. |
sdk/core/core-http/rollup.config.ts
Outdated
| @@ -31,7 +31,8 @@ const nodeConfig = { | |||
| "tslib", | |||
| "tunnel", | |||
| "uuid/v4", | |||
| "xml2js", | |||
| "xml2js",, | |||
| SpanOptions, | ||
| SpanContext, | ||
| NoOpSpan | ||
| } from "../../lib/coreHttp"; |
There was a problem hiding this comment.
Might want to run rush format (or Prettier) on all these files because these blocks will just get expanded again once someone else does.
There was a problem hiding this comment.
These formatting changes have been happening because I have the VS Code prettier extension turned on and it will happily expand/de-expand based on line length. It doesn't seem to prefer one canonical way.
Sidebar: You've reminded me that I've been meaning to fix rush format, it's currently broken because @azure/core-arm doesn't have a format command.
Also when I tried to run format locally in core-http it turns out we didn't devdepend on prettier for this repo:
c:\src\azure-sdk-for-js\sdk\core\core-http>npm run format
> @azure/[email protected] format c:\src\azure-sdk-for-js\sdk\core\core-http
> prettier --write --config ../../.prettierrc.json "lib/**/*.ts" "test/**/*.ts" "*.{js,json}"
'prettier' is not recognized as an internal or external command,
operable program or batch file.
I'll tackle this stuff in another PR to avoid noise here. :)
|
One of the reasons (other than rollup/rush) that we exported all tracing stuff from core-http was because we were worried about the scenario where the version of core-tracing used by the customer and the one used by our libraries could be different in which case the tracer set by the customer may not be accessible by the libraries |
If you don't mark a module as
So it's true that you could import a different version, but it's already internally important that every service package uses the same version, so I think asking users to match the version they import to the SDK they're using isn't a very difficult requirement. We can document this in the README to make it more obvious and maybe log some errors/warnings if you duplicate the package accidentally. Even if we re-export it through core-http, a user could still technically use different competing versions of our service libs that depend on different versions of core-http/core-tracing. They could also import a different version of core-http (unless we re-export again from each service lib.) There's also the problem that amqp things don't import from core-http, so we'd have that split to deal with as well. It's a thorny problem, but I think with good docs and some helpful logging we can avoid most of the foot-guns here.
Yes, I merged this PR with my branch that had storage-queue tracing, added the tracing policy, and got the core-http trace span mixed in with my queue client spans. 👍 I'll make another PR to the storage libs after this goes in to add back the tracingPolicy and update the tests. |
|
There are some merge conflicts that need to be taken care of @richardpark-msft Once this PR is merged, you will need to rebase and tweak your PR #5360 |
core-http was inlining core-tracing, which caused get/setTracer to no longer be a global singleton.
xirzec
force-pushed
the
fixCoreTracingDuplication
branch
from
9bf40f9 to
6a863e8
Compare
Currently we have an issue where core-http is re-exporting a private copy of core-tracing for node, which causes spans to not get created on the proper tracer.
This PR fixes this issue by correctly configuring rollup, which eliminates the need to re-export core-tracing types. The other changes in this PR are having other packages now correctly depend on core-tracing instead of consuming it via core-http.