Sentry-related spans are appearing in dns/net opentelemetry traces

[Bruno-DaSilva]

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

1. Setup SDK as above
2. Run the app, hit an endpoint that triggers a captureException or captureMessage
3. 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:

[Bruno-DaSilva]

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]

Hi, we are already using suppressTracing for 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]

Hi, we are already using suppressTracing for 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.

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]

I have found this:

sentry-javascript/packages/node/src/transports/http.ts

Lines 82 to 86 in aaaaa7d

	// This ensures we do not generate any spans in OpenTelemetry for the transport
	return suppressTracing(() => {
	const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent);
	return createTransport(options, requestExecutor);
	});

Which calls two functions, createRequestExecutor and createTransport. 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

return function makeRequest(request: TransportRequest): Promise<TransportMakeRequestResponse> {

and:

sentry-javascript/packages/core/src/transports/base.ts

Lines 45 to 47 in aaaaa7d

	const flush = (timeout?: number): PromiseLike<boolean> => buffer.drain(timeout);
	function send(envelope: Envelope): PromiseLike<TransportMakeRequestResponse> {

sentry-javascript/packages/core/src/transports/base.ts

Lines 108 to 111 in aaaaa7d

	return {
	send,
	flush,
	};

Now I think the problem is in how we're trying to suppressTracing here. 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 flush function? 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

	if (isSentryRequestUrl(url, getClient())) {
	return true;
	}

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]

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