-
Notifications
You must be signed in to change notification settings - Fork 807
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
Document that ZoneContextManager doesn't work with async/await code that targets ES2017+ #1502
Comments
Hmm. I think at the very least we need some docs updates here, but I don't think this is actually a bug as there are no behavior changes requested. Regarding the suggested docs, I think we should suggest that if users are using the Zone context manager, they should transpile to ES2015. If that is not an option, explicit context propagation is the fallback choice. The main reason for this is that transpiling doesn't require code changes to the user application in order to make tracing work. |
Ah you're right I did not specify a change in behavior beyond updating the documentation. I can rectify this. First, I don't know whether or not opentelemetry-js should be targeting ES2017. This seems like it might be wrong? Second, I don't think these APIs should be reliant on implicit global context to generate properly nested traces. Many of the functions seem to use a default parameter of |
It's also (imo) clearly a bug that tracing in the browser is broken by default in modern browsers when using modern JavaScript. Since wrong nesting for traces can mean missing or faulty propagation values, this bug can affect server side traces that use values from the browser implementation. |
We are going to document that an end user application using the Zone context manager should target ES2015. If that is not possible, we will recommend users manually propagate context. Dropping implicit context management is not feasible as it would possibly violate spec, and much of the code is shared with node.js which propagates context without problems. See related Zone.js/angular issue angular/angular#31730 |
ZoneContextManager does not propagate context when entering native async/await. open-telemetry/opentelemetry-js#1502 angular/angular#31730 Transpiling to es2015 doesn't sound great. Looking for alternatives. Maybe `Dexiejs` promise which promises to work.
## Which problem is this PR solving? Adds documentation on supplying your own context manager References: - https://opentelemetry.io/docs/languages/js/context/#context-manager - https://github.com/open-telemetry/opentelemetry-js/tree/main/packages/opentelemetry-context-zone-peer-dep#installation - open-telemetry/opentelemetry-js#1502 - open-telemetry/opentelemetry-js#5104 - open-telemetry/opentelemetry-js#3030 ## How to verify that this has the expected result read the docs
ZoneContextManager
is based on zone.js, which is a part of the Angular project.Unfortunately, zone.js can't track context within an
AsyncFunction
in the web browser: angular/angular#31730If my understanding of the OpenTelemetry APIs and documentation is correct, then this fact is probably very surprising to many OpenTelemetry users.
As an OpenTelemetry user, my choices seem to be:
(I'm not ok with this)
async
/await
(I don't love
.then
and})
staircases enough to do this)(makes debugging very challenging)
(feels wrong since it makes it very expensive to switch a codepath from sync to async)
(winner winner chicken dinner)
Based on my reading of the associated issue threads, it seems unlikely that zone.js will ever be able to support ES2017+ async/await. I can add these as links if they'd be helpful to others.
Assuming I'm not missing anything, I think the OpenTelemetry documentation should be updated to highlight this (major) shortcoming of
ZoneContextManager
. This project might even consider advocating for users to just start using explicit context propagation in the first place.In the medium term it might make sense to drop implicit context propagation altogether.
The text was updated successfully, but these errors were encountered: