-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(otel): Ignore outgoing Sentry HTTP requests from otel integration #6116
Conversation
Need to make sure to sync this with https://github.com/getsentry/sentry-javascript/pull/6114/files, as that also touched the map! |
@@ -22,7 +26,13 @@ describe('SentrySpanProcessor', () => { | |||
let spanProcessor: SentrySpanProcessor; | |||
|
|||
beforeEach(() => { | |||
hub = new Hub(); | |||
const client = new NodeClient({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AbhiPrasad I see here: https://github.com/getsentry/sentry-javascript/pull/6114/files#diff-c10f2a296bc2f7b1e2908434ede6d48588c8add094f6270465f06fd41b35ac34R85 you added a mock client, should we do the same here? I wasn't quite sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind either way - using a mock client was faster for me, but I'm fine with creating a NodeClient here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tests!
size-limit report 📦
|
With the basic setup of the SpanProcessor, you can get into an infinite loop quickly:
@opentelemetry/instrumentation-http
)The "cleanest" way to avoid this is to setup your auto-instrumentation to skip Sentry requests, e.g.:
However, this also means you need to make sure to cover any potential place where this is auto-instrumented, and it also makes the setup experience for users harder than necessary.
Instead, this PR adds some handling to handle these cases automatically.
Actual solution
Any transaction/span that we identify as being an HTTP request to the configured Sentry DSN is never finished, and thus never sent to Sentry.
Known downsides
Any theoretical child spans of the Sentry HTTP request span will also never be sent to Sentry. This shouldn't actually happen that much, or be of high relevance if it happens, but it is theoretically possible.
Explanation
At least as of now, there is simply no way in
onStart
of the SpanProcessor to access the attributes, as they are only set afteronStart
has been called. Because of this, any behavior where we handle this/properly filter this out before the span ends is not possible to implement.We will need to do some testing to ensure there are no memory leaks (it should be fine, but to be sure...)
ref #6112
References