[core-tracing] support sharing state between CJS and ESM #28631
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maorleger
requested review from
xirzec,
jeremymeng and
joheredi
as code owners
|
This is where I landed, would love any feedback. I will be looking at tests and working through any tracing test failures |
mpodwysocki
reviewed
Feb 21, 2024
|
API change check API changes are not detected in this pull request. |
xirzec
reviewed
Feb 21, 2024
maorleger
force-pushed
the
mleger-fix-tracing
branch
3 times, most recently
from
f8860fa to
269871d
Compare
|
/azp run js - monitor-query - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
maorleger
changed the title
maorleger
commented
Feb 22, 2024
| @@ -13,6 +14,9 @@ export default defineConfig({ | |||
| toFake: ["setTimeout", "Date"], | |||
| }, | |||
| watch: false, | |||
| alias: { | |||
| "../commonjs/state.js": resolve("./src/state-cjs.cts"), | |||
There was a problem hiding this comment.
Not quite scalable IMO but I will be exploring alternatives in a follow-up PR
There was a problem hiding this comment.
I think that's fine for now since Node.js is really the only place it applies.
mpodwysocki
reviewed
Feb 22, 2024
mpodwysocki
approved these changes
Feb 22, 2024
3 tasks
maorleger
added a commit
that referenced
this pull request
Mar 8, 2024
### Packages impacted by this PR @azure/core-client ### Issues associated with this PR N/A, follow up on #28631 ### Describe the problem that is addressed by this PR If you are exporting both CommonJS and ESM forms of a package, then it is possible for both versions to be loaded at run-time. However, the CommonJS build is a different module from the ESM build, and thus a different thing from the point of view of the JavaScript interpreter in Node.js. https://github.com/isaacs/tshy/blob/main/README.md#dual-package-hazards tshy handles this by building programs into separate folders and treats "dual module hazards" as a fact of life. One of the hazards of dual-modules is shared module-global state. In core-clientwe have a module-global operationRequestMap that is used for deserializing. In order to ensure it works in this dual-package world we must use one of multiple-recommended workarounds. In this case, the tshy documentation provides a solution to this with a well-documented path forward. This is what is implemented here. Please refer to https://github.com/isaacs/tshy/blob/main/README.md#module-local-state for added context ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? The obvious alternative is to just not do anything since tests have not been failing; however, that seems risky. While _this_ particular issue has not come up in tests, a similar one came up for core-tracing. I am open to just _not_ doing anything of course - I love not adding code so just give me a reason!
sofiar-msft
pushed a commit
to sofiar-msft/azure-sdk-for-js
that referenced
this pull request
Apr 11, 2024
### Packages impacted by this PR @azure/core-tracing ### Issues associated with this PR Resolves Azure#28619 ### Describe the problem that is addressed by this PR If you are exporting both CommonJS and ESM forms of a package, then it is possible for both versions to be loaded at run-time. However, the CommonJS build is a different module from the ESM build, and thus a different thing from the point of view of the JavaScript interpreter in Node.js. > https://github.com/isaacs/tshy/blob/main/README.md#dual-package-hazards tshy handles this by building programs into separate folders and treats "dual module hazards" as a fact of life. One of the hazards of dual-modules is shared module-global state. In core-tracing we have a module-global instrumenter that is used for hooking into and lighting up OpenTelemetry based instrumentation. In order to ensure it works in this dual-package world we must use one of multiple-recommended workarounds. In this case, the tshy documentation provides a solution to this with a well-documented path forward. This is what is implemented here. Please refer to https://github.com/isaacs/tshy/blob/main/README.md#module-local-state for added context
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Packages impacted by this PR
@azure/core-tracing
Issues associated with this PR
Resolves #28619
Describe the problem that is addressed by this PR
If you are exporting both CommonJS and ESM forms of a package, then it is possible for both versions to be loaded at run-time.
However, the CommonJS build is a different module from the ESM build, and thus a different thing from the point of view of the
JavaScript interpreter in Node.js.
tshy handles this by building programs into separate folders and treats "dual
module hazards" as a fact of life.
One of the hazards of dual-modules is shared module-global state.
In core-tracing we have a module-global instrumenter that is used for hooking
into and lighting up OpenTelemetry based instrumentation. In order to ensure it
works in this dual-package world we must use one of multiple-recommended
workarounds.
In this case, the tshy documentation provides a solution to this with a
well-documented path forward. This is what is implemented here.
Please refer to
https://github.com/isaacs/tshy/blob/main/README.md#module-local-state for added
//context.
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Alternatively, I considered migrating test-utils to ESM and seeing if that
resolves the issue; however, I do believe that our tests are trying to tell us
something here and that we should resolve the issue so that customers who are
using OTel may not be impacted.
Are there test cases added in this PR? (If not, why?)
Honestly? Wasn't sure how to test this... tshy build breaks when you do things
incorrectly and provides some validation.
Provide a list of related PRs (if any)
Command used to generate this PR: (Applicable only to SDK release request PRs)
Checklists