Fix core-tracing duplication

[xirzec]

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.

[xirzec]

@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.

[daviwil]

Nice! To be honest, I like it better this way. Importing tracing functions and types from core-tracing is much clearer.

[daviwil]

Stray comma at EOL

[daviwil]

Might want to run rush format (or Prettier) on all these files because these blocks will just get expanded again once someone else does.

[xirzec]

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/core-http@1.0.0-preview.3 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. :)

[chradek]

This is awesome!

For my own sanity can you explain why the previous rollup config was causing this issue?

[ramya-rao-a]

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

[chradek]

You mentioned that you tested this with storage. Does that include with the tracing policy? What we had seen before was that the tracing policy would could getTracer and get a NoOpTracer since we were seeing core-tracing loaded twice.

[xirzec]

For my own sanity can you explain why the previous rollup config was causing this issue?

If you don't mark a module as external, then it actually inlines the entire thing. So our dist\coreHttp.node.js contained its own version of core-tracing.

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

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.

You mentioned that you tested this with storage. Does that include with the tracing policy? What we had seen before was that the tracing policy would could getTracer and get a NoOpTracer since we were seeing core-tracing loaded twice.

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.

[ramya-rao-a]

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