Node Compat & AsyncContext

[RichiCoder1]

One of the biggest warts with the Otel sdk today is we can not behave like the Node and Browser versions and automatically patch and flow tracestate through fetch calls. This is partially because there's no garuntees about global state (and it's actively discouraged) and there's no equivalent to async_hooks in Cloudflare Workers.

...

Until now! https://twitter.com/jasnell/status/1633949738516230144?s=20

Cloudflare has added a node_compat option that enables async_hooks just like Node and is planning to implement a brand new proposal to standardize Async state storage in an efficient manner.

As such, I'm planning to try and release a new version of the SDK based on hooks and monkey-patching fetch like other SDKs do and hopefully removing the need to fetch via the SDK.

Warning
This will be a breaking change compared to how it works today

[yacinehmito]

Would it mean that the node_compat option would be required in order to use opentelemetry-sdk-workers? Or just the automatic tracing?

[RichiCoder1]

Would it mean that the node_compat option would be required in order to use opentelemetry-sdk-workers? Or just the automatic tracing?

I'm still on the fence about this and would love to hear any feedback.
Currently we maintain a lot of code specific to not being able to have an asynchronous-friendly context and having to patch fetch at the request level. I'm not personally comfortable with maintaining two simultaneous versions of instrumentation for the SDK, so the guidance would be releasing a new version which requires the node_compat option to be flipped on. At least until the Async Context RFC makes it into workers unflagged.

That said, having talked with a member of the Cloudflare team there should be no downside to flipping on that flag as I understand it.

[yacinehmito]

That said, having talked with a member of the Cloudflare team there should be no downside to flipping on that flag as I understand it.

Indeed, but I feel like this is a choice to be made on the part of the application developer.

The node_compat flag is useful to run code meant for node. It would be disappointing to have to set this flag for something built for Workers. It kind of defeats the purpose.

I'm not personally comfortable with maintaining two simultaneous versions of instrumentation for the SDK

That makes sense. My opinion: dropping the current instrumentation SDK is fine, as long as the SDK is designed so that one can reimplement the API in user space is they don't want node_compat. This way, the requirement won't be for the whole library, but only its instrumentation API. What do you think?

[RichiCoder1]

The node_compat flag is useful to run code meant for node. It would be disappointing to have to set this flag for something built for Workers. It kind of defeats the purpose.

The hope is that eventually the flag would get dropped when the RFC lands in workers, while providing a much better DevUX where you don't have to constantly manually invoke the SDK to instrument everything. So while I agree that it's certainly weird, the node_compat is the bridge for making these sort of problems work while still otherwise respecting the environment that is workers.

dropping the current instrumentation SDK is fine, as long as the SDK is designed so that one can reimplement the API in user space is they don't want node_compat. This way, the requirement won't be for the whole library, but only its instrumentation API. What do you think?

Unfortunately that may not be easily workable. We're shifting assumptions the SDK makes from "there's a per work context that we have to manually flow ourselves" to "there's an async context that we can make automatically flow for each workers call". I don't see a way to make both worth without reimplementing 50% of the SDK basically.

[yacinehmito]

That makes sense.

Let's say that I don't like it, but it is a sensible choice. The fact that there is a standard on the horizon managed to assuage my concerns.

Thanks a lot for laying out your reasoning.

[RichiCoder1]

For sure! This is an unambiguously breaking change and requires an esoteric flag, so I want to make the reasoning is sound. I'll be updating the documentation to outline as such when this work lands.
(maybe I should write an ADR 🤔).

[RichiCoder1]

Official blog post announcement: https://blog.cloudflare.com/workers-node-js-asynclocalstorage/