feat(node): Implement Sentry-specific http instrumentation #13763
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
mydea
commented
Sep 24, 2024
dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
Outdated
Show resolved
Hide resolved
size-limit report 📦
|
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
mydea
added a commit
that referenced
this pull request
Sep 24, 2024
) 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.get` and `http.request` properly.
mydea
force-pushed
the
fn/custom-http-shim
branch
from
8cb19ad
to
a9fde4f
Compare
|
v1.11.1 of |
mydea
force-pushed
the
fn/custom-http-shim
branch
from
b7c8b37
to
00d177d
Compare
mydea
commented
Oct 8, 2024
| @@ -14,8 +14,7 @@ | |||
| }, | |||
| "devDependencies": { | |||
| "@sentry-internal/test-utils": "link:../../../test-utils", | |||
| "@playwright/test": "^1.44.1", | |||
| "wait-port": "1.0.4" | |||
There was a problem hiding this comment.
cleaned this up as this is not needed anymore
mydea
commented
Oct 8, 2024
| @@ -18,7 +18,7 @@ test('AWS Serverless SDK sends events in ESM mode', async ({ request }) => { | |||
| ); | |||
|
|
|||
| child_process.execSync('pnpm start', { | |||
| stdio: 'ignore', | |||
There was a problem hiding this comment.
was debugging a test here for some time, this made it much easier to debug this
Lms24
approved these changes
Oct 8, 2024
timfish
reviewed
Oct 9, 2024
There was a problem hiding this comment.
I'm trying to work out what Node version diagnostics_channel http events were added because they probably make this all possible without touching otel/import-in-the-middle. It has both start and end events for both incoming and outgoing requests.
UPDATE
Yep, just tested and these events are available in v14.17.0
|
Let me know if you want me to have a try at reworking this PR with |
|
Thanks for that Tim, I was not aware this was available for Node 14+! I will give it a quick look to see if I can easily use this instead in this PR, if so I'll update it right away. If it is more complex, maybe we merge this and then update it in a follow up :) |
timfish
approved these changes
Oct 10, 2024
mydea
force-pushed
the
fn/custom-http-shim
branch
from
6bf01c5
to
d0aac35
Compare
|
So for reference, it is not that easy to move this to diagnostics channel, because there we cannot wrap the incoming request in an easy way to ensure async local storage has the correct isolation scope etc. So I'm going to merge this like this, we can investigate in a follow up if we can refactor it! |
mydea
commented
Oct 11, 2024
| @@ -25,6 +10,7 @@ type HttpOptions = Parameters<typeof originalHttpIntegration>[0]; | |||
| export const httpIntegration = ((options: HttpOptions = {}) => { | |||
| return originalHttpIntegration({ | |||
| ...options, | |||
| _instrumentation: RemixHttpIntegration, | |||
| // We disable incoming request spans here, because otherwise we'd end up with duplicate spans. | |||
| disableIncomingRequestSpans: true, | |||
There was a problem hiding this comment.
cc @lforst also relevant for Next after we merge this
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.
alexandresoro
pushed a commit
to alexandresoro/ouca
that referenced
this pull request
Oct 21, 2024
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@sentry/node](https://github.com/getsentry/sentry-javascript/tree/master/packages/node) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.34.0` -> `8.35.0`](https://renovatebot.com/diffs/npm/@sentry%2fnode/8.34.0/8.35.0) | | [@sentry/react](https://github.com/getsentry/sentry-javascript/tree/master/packages/react) ([source](https://github.com/getsentry/sentry-javascript)) | dependencies | minor | [`8.34.0` -> `8.35.0`](https://renovatebot.com/diffs/npm/@sentry%2freact/8.34.0/8.35.0) | --- ### Release Notes <details> <summary>getsentry/sentry-javascript (@​sentry/node)</summary> ### [`v8.35.0`](https://github.com/getsentry/sentry-javascript/blob/HEAD/CHANGELOG.md#8350) [Compare Source](getsentry/sentry-javascript@8.34.0...8.35.0) ##### Beta release of the official Nuxt Sentry SDK This release marks the beta release of the `@sentry/nuxt` Sentry SDK. For details on how to use it, check out the [Sentry Nuxt SDK README](https://github.com/getsentry/sentry-javascript/tree/develop/packages/nuxt). Please reach out on [GitHub](https://github.com/getsentry/sentry-javascript/issues/new/choose) if you have any feedback or concerns. - **feat(nuxt): Make dynamic import() wrapping default ([#​13958](getsentry/sentry-javascript#13958 (BREAKING) - **feat(nuxt): Add Rollup plugin to wrap server entry with `import()` ([#​13945](getsentry/sentry-javascript#13945 **It is no longer required to add a Node `--import` flag. Please update your start command to avoid initializing Sentry twice (BREAKING CHANGE).** The SDK will now apply modifications during the build of your application to allow for patching of libraries during runtime. If run into issues with this change, you can disable this behavior in your `nuxt.config.ts` and use the `--import` flag instead: ```js sentry: { dynamicImportForServerEntry: false; } ``` - **feat(nuxt): Respect user-provided source map generation settings ([#​14020](getsentry/sentry-javascript#14020 We now require you to explicitly enable sourcemaps for the clientside so that Sentry can un-minify your errors. We made this change so source maps aren't accidentally leaked to the public. Enable source maps on the client as follows: ```js export default defineNuxtConfig({ sourcemap: { client: true, }, }); ``` - feat(nuxt): Log server instrumentation might not work in dev ([#​14021](getsentry/sentry-javascript#14021)) - feat(nuxt): Add Http `responseHook` with `waitUntil` ([#​13986](getsentry/sentry-javascript#13986)) ##### Important Changes - **feat(vue): Add Pinia plugin ([#​13841](getsentry/sentry-javascript#13841 Support for [Pinia](https://pinia.vuejs.org/) is added in this release for `@sentry/vue`. To capture Pinia state data, add `createSentryPiniaPlugin()` to your Pinia store: ```javascript import { createPinia } from 'pinia'; import { createSentryPiniaPlugin } from '@​sentry/vue'; const pinia = createPinia(); pinia.use(createSentryPiniaPlugin()); ``` - **feat(node): Implement Sentry-specific http instrumentation ([#​13763](getsentry/sentry-javascript#13763 This change introduces a new `SentryHttpInstrumentation` to handle non-span related HTTP instrumentation, allowing it to run side-by-side with OTel's `HttpInstrumentation`. This improves support for custom OTel setups and avoids conflicts with Sentry's instrumentation. Additionally, the `spans: false` option is reintroduced for `httpIntegration` to disable span emission while still allowing custom `HttpInstrumentation` instances (`httpIntegration({ spans: false })`). - **feat(core): Make stream instrumentation opt-in ([#​13951](getsentry/sentry-javascript#13951 This change adds a new option `trackFetchStreamPerformance` to the browser tracing integration. Only when set to `true`, Sentry will instrument streams via fetch. ##### Other Changes - feat(node): Expose `suppressTracing` API ([#​13875](getsentry/sentry-javascript#13875)) - feat(replay): Do not log "timeout while trying to read resp body" as exception ([#​13965](getsentry/sentry-javascript#13965)) - chore(node): Bump `@opentelemetry/instrumentation-express` to `0.43.0` ([#​13948](getsentry/sentry-javascript#13948)) - chore(node): Bump `@opentelemetry/instrumentation-fastify` to `0.40.0` ([#​13983](getsentry/sentry-javascript#13983)) - fix: Ensure type for `init` is correct in meta frameworks ([#​13938](getsentry/sentry-javascript#13938)) - fix(core): `.set` the `sentry-trace` header instead of `.append`ing in fetch instrumentation ([#​13907](getsentry/sentry-javascript#13907)) - fix(module): keep version for node ESM package ([#​13922](getsentry/sentry-javascript#13922)) - fix(node): Ensure `ignoreOutgoingRequests` of `httpIntegration` applies to breadcrumbs ([#​13970](getsentry/sentry-javascript#13970)) - fix(replay): Fix onError sampling when loading an expired buffered session ([#​13962](getsentry/sentry-javascript#13962)) - fix(replay): Ignore older performance entries when starting manually ([#​13969](getsentry/sentry-javascript#13969)) - perf(node): Truncate breadcrumb messages created by console integration ([#​14006](getsentry/sentry-javascript#14006)) Work in this release was contributed by [@​ZakrepaShe](https://github.com/ZakrepaShe) and [@​zhiyan114](https://github.com/zhiyan114). Thank you for your contributions! </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4xMjguNiIsInVwZGF0ZWRJblZlciI6IjM4LjEyOC42IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiXX0=--> Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/251 Reviewed-by: Alexandre Soro <code@soro.dev> Co-authored-by: renovate <renovate@git.tristess.app> Co-committed-by: renovate <renovate@git.tristess.app>
Closed
3 tasks
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.
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
HttpInstrumentationfor Sentry-specific (read: not span-related) things. This made it tricky/annoying for users with a custom OTEL setup that may includeHttpInstrumentationto 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
wrapmethod 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:
With this change, in errors only mode you really do not need our instance of the default
HttpInstrumentationanymore 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: falseforhttpIntegration({ 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.