Skip to content
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

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

mydea
Copy link
Member

@mydea mydea commented Nov 2, 2022

With the basic setup of the SpanProcessor, you can get into an infinite loop quickly:

  • You have some OTEL auto-instrumentation set up
  • This auto-instrumentation captures outgoing HTTP requests (e.g. @opentelemetry/instrumentation-http)
  • You setup the Sentry<>OTEL integration
  • For any OTEL span created, we capture this to Sentry
  • This is sent to Sentry as an outgoing HTTP request
  • Which again is captured by the OTEL auto-instrumentation, ....

The "cleanest" way to avoid this is to setup your auto-instrumentation to skip Sentry requests, e.g.:

const sdk = new opentelemetry.NodeSDK({
  traceExporter: new OTLPTraceExporter(),
  instrumentations: [ 
    getNodeAutoInstrumentations({
      // load custom configuration for http instrumentation
      '@opentelemetry/instrumentation-http': {
        ignoreOutgoingRequestHook: (request) => {
          const host = request.host || request.hostname;
          // Ignore Sentry API calls
          return host.indexOf('.sentry.io') > -1;
        }
      },
    }), 
  ],
  spanProcessor: new SentrySpanProcessor()
})

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 after onStart 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

@mydea
Copy link
Member Author

mydea commented Nov 2, 2022

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({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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.

Copy link
Member

@AbhiPrasad AbhiPrasad left a 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!

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.5 KB (+0.03% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 60.35 KB (0%)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.13 KB (+0.02% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 53.68 KB (0%)
@sentry/browser - Webpack (gzipped + minified) 19.87 KB (0%)
@sentry/browser - Webpack (minified) 65.09 KB (0%)
@sentry/react - Webpack (gzipped + minified) 19.89 KB (0%)
@sentry/nextjs Client - Webpack (gzipped + minified) 45.79 KB (0%)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.25 KB (+0.01% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.63 KB (+0.01% 🔺)

@mydea mydea merged commit b55dd07 into master Nov 2, 2022
@mydea mydea deleted the fn/otel-skip-sentry branch November 2, 2022 15:50
@AbhiPrasad AbhiPrasad added this to the OpenTelemetry Support milestone Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants