fix(node): Ensure node-fetch does not emit spans without tracing #13765
Conversation
This file contains hidden or 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
Contributor
size-limit report 📦
|
timfish approved these changes Sep 24, 2024
Collaborator
timfish left a comment
timfish left a comment There was a problem hiding this comment.
Oh no!
The classic "I deleted the code and all the tests pass so it must not be needed" 😬
mydea added a commit that referenced this pull request Oct 11, 2024
This PR is a pretty big change, but it should help us to make custom OTEL support way better/easier to understand in the future. The fundamental problem this PR is trying to change is the restriction that we rely on our instance of `HttpInstrumentation` for Sentry-specific (read: not span-related) things. This made it tricky/annoying for users with a custom OTEL setup that may include `HttpInstrumentation` to get things working, because they may inadvertedly overwrite our instance of the instrumentation (because there can only be a single monkey-patch per module in the regular instrumentations), leading to hard-to-debug and often subtle problems. This PR fixes this by splitting out the non-span related http instrumentation code into a new, dedicated `SentryHttpInstrumentation`, which can be run side-by-side with the OTEL instrumentation (which emits spans, ...). We make this work by basically implementing our own custom, minimal `wrap` method instead of using shimmer. This way, OTEL instrumentation cannot identify the wrapped module as wrapped, and allow to wrap it again. While this is _slightly_ hacky and also means you cannot unwrap the http module, we do not generally support this for the Sentry SDK anyhow. This new Instrumentation does two things: 1. Handles automatic forking of the isolation scope per incoming request. By using our own code, we can actually make this much nicer, as we do not need to retrospectively update the isolation scope anymore, but instead we can do this properly now. 2. Emit breadcrumbs for outgoing requests. With this change, in errors only mode you really do not need our instance of the default `HttpInstrumentation` anymore at all, you can/should just provide your own if you want to capture http spans in a non-Sentry environment. However, this is sadly a bit tricky, because up to now we forced users in this scenario to still use our Http instance and avoid adding their own (instead we allowed users to pass their Http instrumentation config to our Http integration). This means that if we'd simply stop adding our http instrumentation instance when tracing is disabled, these users would stop getting otel spans as well :/ so we sadly can't change this without a major. Instead, I re-introduced the `spans: false` for `httpIntegration({ spans: false })`. When this is set (which for now is opt-in, but probably should be opt-out in v9) we will only register SentryHttpInstrumentation, not HttpInstrumentation, thus not emitting any spans. Users can add their own instance of HttpInstrumentation if they care. One semi-related thing that I noticed while looking into this is that we incorrectly emitted node-fetch spans in errors-only mode. This apparently sneaked in when we migrated to the new undici instrumentation. I extracted this out into a dedicated PR too, but the changes are in this too because tests were a bit fucked up otherwise. On top of #13765 This also includes a bump of import-in-the-middle to 1.11.2, as this includes a fix to properly allow double-wrapping ESM modules. billyvg pushed a commit that referenced this pull request Oct 17, 2024
This PR is a pretty big change, but it should help us to make custom OTEL support way better/easier to understand in the future. The fundamental problem this PR is trying to change is the restriction that we rely on our instance of `HttpInstrumentation` for Sentry-specific (read: not span-related) things. This made it tricky/annoying for users with a custom OTEL setup that may include `HttpInstrumentation` to get things working, because they may inadvertedly overwrite our instance of the instrumentation (because there can only be a single monkey-patch per module in the regular instrumentations), leading to hard-to-debug and often subtle problems. This PR fixes this by splitting out the non-span related http instrumentation code into a new, dedicated `SentryHttpInstrumentation`, which can be run side-by-side with the OTEL instrumentation (which emits spans, ...). We make this work by basically implementing our own custom, minimal `wrap` method instead of using shimmer. This way, OTEL instrumentation cannot identify the wrapped module as wrapped, and allow to wrap it again. While this is _slightly_ hacky and also means you cannot unwrap the http module, we do not generally support this for the Sentry SDK anyhow. This new Instrumentation does two things: 1. Handles automatic forking of the isolation scope per incoming request. By using our own code, we can actually make this much nicer, as we do not need to retrospectively update the isolation scope anymore, but instead we can do this properly now. 2. Emit breadcrumbs for outgoing requests. With this change, in errors only mode you really do not need our instance of the default `HttpInstrumentation` anymore at all, you can/should just provide your own if you want to capture http spans in a non-Sentry environment. However, this is sadly a bit tricky, because up to now we forced users in this scenario to still use our Http instance and avoid adding their own (instead we allowed users to pass their Http instrumentation config to our Http integration). This means that if we'd simply stop adding our http instrumentation instance when tracing is disabled, these users would stop getting otel spans as well :/ so we sadly can't change this without a major. Instead, I re-introduced the `spans: false` for `httpIntegration({ spans: false })`. When this is set (which for now is opt-in, but probably should be opt-out in v9) we will only register SentryHttpInstrumentation, not HttpInstrumentation, thus not emitting any spans. Users can add their own instance of HttpInstrumentation if they care. One semi-related thing that I noticed while looking into this is that we incorrectly emitted node-fetch spans in errors-only mode. This apparently sneaked in when we migrated to the new undici instrumentation. I extracted this out into a dedicated PR too, but the changes are in this too because tests were a bit fucked up otherwise. On top of #13765 This also includes a bump of import-in-the-middle to 1.11.2, as this includes a fix to properly allow double-wrapping ESM modules. 
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.
Found this while working on #13763.
Oops, the node-otel-without-tracing E2E test was not running on CI, we forgot to add it there - and it was actually failing since we switched to the new undici instrumentation :O
This PR ensures to add it to CI, and also fixes it. The main change for this is to ensure we do not emit any spans when tracing is disabled, while still ensuring that trace propagation works as expected for this case.
I also pulled some general changes into this, which ensure that we patch both
http.getandhttp.requestproperly.