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

Using zone.js in otel is a bit risky and buggy #612

Open
myieye opened this issue Feb 26, 2024 · 4 comments
Open

Using zone.js in otel is a bit risky and buggy #612

myieye opened this issue Feb 26, 2024 · 4 comments
Labels
bug Something isn't working OpenTelemetry owner: Tim, Kevin
Milestone

Comments

@myieye
Copy link
Contributor

myieye commented Feb 26, 2024

I finally understand this warning about using the ZoneContextManager for OTEL tracing in the browser:

Please note that due to an issue with zone.js, the ZoneContextManager does not work with JS code targeting ES2017+. In order to use the ZoneContextManager, please transpile back to ES2015.

And I'm pretty sure it explains this issue that @rmunn was having:

  • Add a .refine() call to the project code field
    • Zod docs say that refine() can be async
    • But the async refine call didn't produce a validation error when it returned false
    • When I returned Promise.fulfill(false) from a sync refine call, it produced a validation error
    • But when I returned fetch('/api/...').then(return false), i.e. a Promise that isn't already fulfilled, no validation error showed up in the form

I don't think we're currently having any "real" issues, like this one. We don't use afterNavigate, but there could be other affected hooks etc.

Essentially, ZoneContextManager (or zone.js more specifically) doesn't support native Promises or async/await syntax (because that syntax using native Promises). So, its tracking of asynchronous operations gets buggy in those scenarios. And then I guess weird things can happen.

Here's the issue tracking zone.js' support of async/await syntax. Essentially it sounds like it will never happen.

Our options:

  1. Leave things the way they are:
    ➕ we benefit from ZoneContextManager handling the async APIs that it can track/instrument
    ➖ weird bugs could arise
  2. Try to transpile back to ES2015 out async/await syntax using babel-plugin-transform-async-to-promises. Which is apparently how Angular solves it
    ZoneContextManager would be happy and we might even get better traces (e.g. this might actually end up in the context of a trace)
    ➖ It's trickier to do this with Svelte than with other non-compiler based frameworks
    ➖ Transpiled code results in uglier/less readable stack traces
  3. Drop ZoneContextManager
    ➕ The risk is gone
    ➖ Some of our traces might fall apart
@myieye myieye added bug Something isn't working OpenTelemetry owner: Tim, Kevin labels Feb 26, 2024
@megahirt
Copy link
Contributor

Leave things the way they are:

➕ There is also the possibility that zone.js will eventually support ES2017+ if we do nothing, and then presumably things will be more stable.

@myieye
Copy link
Contributor Author

myieye commented Feb 27, 2024

I updated the description. It sounds very likely that zone.js will never be able to support async/await and people are transpiling (only) that syntax out. The most promising alternative on the horizon seems to be an 8 year old language proposal that probably won't make it into ES any time soon 😆.

@megahirt
Copy link
Contributor

Wow, interesting stuff. Having read about the problem more, I can see how zone.js will never be able to support async/await. In ES proposal land, I suppose that 8 years is not horrible, or is it?

@hahn-kev
Copy link
Collaborator

Interesting problem. Dotnet actually had this problem and solved it with AsyncLocal which sounds like the async contexts is their version of that. It sounds like it would be worth checking out droping ZoneContextManager and see how that works for us. Maybe it won't but It's not clear what the tradeoffs are right now.

@hahn-kev hahn-kev added this to the vNext milestone Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working OpenTelemetry owner: Tim, Kevin
Projects
None yet
Development

No branches or pull requests

3 participants