Repository navigation
Sentry-related spans are appearing in dns/net opentelemetry traces #13466
Description
Is there an existing issue for this?
- I have checked for existing issues https://github.com/getsentry/sentry-javascript/issues
- I have reviewed the documentation https://docs.sentry.io/
- I am using the latest SDK release https://github.com/getsentry/sentry-javascript/releases
How do you use Sentry?
Sentry Saas (sentry.io)
Which SDK are you using?
@sentry/node
SDK Version
8.26.0
Framework Version
Node 20.12.2
Link to Sentry event
No response
Reproduction Example/SDK Setup
Sentry.init({
dsn: process.env.SENTRY_DSN,
skipOpenTelemetrySetup: true
});// We want 100% sample rate but without telling sentry to init tracing (rolling our own tracing, using sentry for errors only).
export class CustomSentrySampler implements Sampler {
shouldSample(
context: Context,
_traceId: string,
_spanName: string,
_spanKind: SpanKind,
attributes: SpanAttributes,
_links: Link[],
) {
const decision = SamplingDecision.RECORD_AND_SAMPLED;
// wrap the result, necessary for sentry
return SentryOtel.wrapSamplingDecision({
decision,
context,
spanAttributes: attributes,
});
}
toString() {
return CustomSentrySampler.name;
}
}
// can use SentrySampler if you init sentry with sample rate instead
const provider = new NodeTracerProvider({ sampler: new CustomSentrySampler(), });
// Only required if you are sending traces up to sentry
// provider.addSpanProcessor(new SentrySpanProcessor());
// an alternative exporter if you don't want to send traces to sentry
provider.addSpanProcessor(new OTLPTraceExporter({
url: TRACING_EXPORTER_URL,
}));
const propagator = SentryPropagator();
const contextManager = SentryContextManager();
// register additional instrumentations outside of what sentry uses by default.
// For this bug report, just adding net/dns instrumentations.
registerInstrumentations({
tracerProvider: provider,
instrumentations: [
new NetInstrumentation(),
new DnsInstrumentation(),
],
});
provider.register({ propagator, contextManager });Steps to Reproduce
- Setup SDK as above
- Run the app, hit an endpoint that triggers a captureException or captureMessage
- Check the trace for this request
Expected Result
The trace should NOT have any sentry related spans in it
Actual Result
Despite the @opentelemetry/instrumentation-http ignoring (not creating spans for) the sentry outgoing requests, the dns and net instrumentations still appear in the trace:
Activity
[-]Sentry-related spans should not appear in opentelemetry traces[/-][+]Sentry-related spans are appearing in dns/net opentelemetry traces[/+]bruno-dasilva commentedon Aug 26, 2024
Hopefully it's clear that this is behaviour that causes spans to be extra spammy, so we should find a way around this.
This is because @opentelemetry/instrumentation-http does not suppress tracing on outgoing ignored requests.
I'm hoping (if y'all agree with this approach) that this feature request in instrumentation-http can be pushed for: open-telemetry/opentelemetry-js#4926
It asks for a config flag to suppress tracing on ignored outgoing requests, to prevent this issue. I figure then the Sentry SDK can either let a user pass that as a custom flag to the underlying instrumentation (no change required in sentry SDK) or can simply enable the flag by default (might be considered a breaking change, up to y'all).
lforst commentedon Aug 27, 2024
Hi, we are already using
suppressTracingfor SDK requests which makes this look like a bug in the dns/net instrumentation. I am not sure if a feature request makes sense in that case.bruno-dasilva commentedon Aug 27, 2024
Hi Luca - could you point me to where this happens in the Sentry SDK? Due to the way instrumentation-http is designed there's no place to suppress tracing that would affect children in the various hooks.
EDIT: found it, see next comment :)
bruno-dasilva commentedon Aug 27, 2024
I have found this:
sentry-javascript/packages/node/src/transports/http.ts
Lines 82 to 86 in aaaaa7d
Which calls two functions,
createRequestExecutorandcreateTransport. In both these functions we do not perform the actual sending of any http calls, we simply create callbacks that will be called later:sentry-javascript/packages/node-experimental/src/transports/http.ts
Line 125 in 603d460
and:
sentry-javascript/packages/core/src/transports/base.ts
Lines 45 to 47 in aaaaa7d
sentry-javascript/packages/core/src/transports/base.ts
Lines 108 to 111 in aaaaa7d
Now I think the problem is in how we're trying to
suppressTracinghere. The suppression of tracing is happening JUST in the creation of these callbacks, NOT during the execution of these callbacks. So I don't think we are suppressing tracing at all when sending the outgoing http requests.I think you'd probably have to suppressTracing(...) within the
flushfunction? Assuming that's the only place we actually send the http reqs.I think the reason we haven't noticed this so far is because we ignore sentry requests in the http integration:
sentry-javascript/packages/node/src/integrations/http.ts
Lines 105 to 107 in aaaaa7d
While that prevents the http spans from being created, it doesn't suppress tracing to catch the dns/net spans (hence the original otel feature request).
SO with all that said, if we can fix how we call suppressTracing in the sentry SDK, I agree that the otel feature request wouldn't be required.
lforst commentedon Aug 28, 2024
I think you're absolutely right. TBH looking at the code retroactively, I would have probably expected the AsyncContext for trace suppression to be preserved down to the code where we execute, but looking a bit closer it makes sense that it is not suppressed because we call that code from a completely different async context.
I opened a PR that should hopefully fix this #13491
fix(node): Suppress tracing for transport request execution rather th…