[tracing] Separate span creation from header propagation

[lobsterkatie]

Note: Whether or not we actually implement this change immediately, we need to make a decision here quite soon, before other SDKs start implementing this.

Current Situation:

Currently, the traceOrigins and shouldCreateSpanForRequest options for the BrowserTracing integration control two things about outgoing http requests, based on the request destination: whether to create a span, and whether to send tracing data in the sentry-trace and baggage headers. (The difference between the two is that shouldCrateSpanForRequest is stricter - it always filters out requests filtered by tracingOrigins, but can also filter out others.)

The problem with this is that there may be outgoing requests which should be represented by a span, but which shouldn't have headers attached (for CORS reasons, for example), and right now that's not possible. We should divorce these two concerns, and allow those decisions to be made separately by the SDK, based on two different options set by the user. We should also bring this functionality to the Node SDK, where nothing similar currently exists.

Known constraint:

We don't want span creation and header attachment be wholly independent, because span creation make sense as a prerequisite for header attachment. (The alternative is a situation where it's possible to propagate headers but not create a span. In that case, the headers would have no parent span to use, and that would break the link between the parent transaction making the http request and the child transaction handling that request.)

Proposal:

• Add a shouldAttachTracingHeadersToRequest option, which will allow control over which outgoing requests traced with a span should include tracing headers.
• Make it clear that shouldCreateSpanForRequest, while it has an influence on header attachment, is not actually for controlling headers. (The default would be to attach headers to any outgoing request for which there's a span, but that would be a default for shouldAttachTracingHeadersToRequest behavior, not for shouldCreateSpanForRequest behavior.)
• Think about deprecating tracingOrigins, because
  • it's not clear which of these concerns it's meant to address, and
  • though I get where the name comes from ("origin" = "domain" in tech-speak), as a way to filter on destination, any name involving "origin" is awfully confusing, given that in regular English, "origin" = where something comes from, not where it's going.
• Make all of this work in @sentry/node as well as @sentry/browser.

Potentially related to https://github.com/getsentry/develop/issues/611.

[souredoutlook]

Hi @lobsterkatie

Just want to add the following problem statement and suggestion to the thread for visibility:

**** Problem Statement****

We are trying to implement a preload hint for our site with a <link rel="preload" as="fetch"> hint.

Unfortunately, Chrome will not use a preloaded asset unless the request headers match, and the Sentry Tracing Integration adds a sentry-trace header to the eventual real fetch request. The request is otherwise identical, so it should consume the fetch preload, but because of the added sentry-trace header, the match fails, the browser issues a warning that a matching preload was found, but could not be used; and an additional fetch request is made anyway.

Feedback

It would be great if the BrowserTracing configuration object accepted something like shouldAddTracing: (url) => ... similar to the shouldCreateSpanForRequest configuration option, to allow passing a function to determine when to include tracing headers, in addition to the tracingOrigins array.

[timfish]

Just to clarify and make sure I've understood correctly...

An optional shouldAttachTracingHeadersToRequest function gets added to RequestInstrumentationOptions and if users supply this function, tracingOrigins should be ignored and shouldAttachTracingHeadersToRequest becomes the sole means of determining if tracing headers should be attached?

[lobsterkatie]

Just to clarify and make sure I've understood correctly...

An optional shouldAttachTracingHeadersToRequest function gets added to RequestInstrumentationOptions and if users supply this function, tracingOrigins should be ignored and shouldAttachTracingHeadersToRequest becomes the sole means of determining if tracing headers should be attached?

Thank you for clarifying! It's a good thing you did, because things have changed quite a bit since I first wrote up the issue, and I didn't know until now that it had been revived. I've rewritten the description so that it's hopefully clearer and so it reflects the new goals of this project. LMK if you still have questions after reading the updated version.

[timfish]

so that it's hopefully clearer and so it reflects the new goals of this project

Wow, thanks. That's a pretty comprehensive summary!

[timfish]

Default to always creating a span, regardless of tracingOrigins, unless shouldCreateSpanForRequest is defined and returns

Is this considered a non-breaking change since it fixes broken behaviour?

Rather than edit your long post, I've copied the tasks here and I'll create a PR for each and add the references:

Browser

• [x] https://github.com/getsentry/sentry-javascript/pull/6039 and https://github.com/getsentry/sentry-javascript/pull/6079
  • Default to always creating a span, regardless of tracingOrigins, unless shouldCreateSpanForRequest is defined and returns false
• [x] https://github.com/getsentry/sentry-javascript/pull/6080
  • Add tracePropagationTargets option with same default value as tracingOrigins
  • Add headers if and only if there's a match to either tracingOrigins or tracePropogationTargets (unless shouldCreateSpanForRequest is defined and returns false, in which case no headers should be added, regardless of tracingOrigins or tracePropogationTargets)
• [x] https://github.com/getsentry/sentry-javascript/pull/6176
  • Mark tracingOrigins as deprecated
  • Fix logging re: tracingOrigins to use tracePropagationTargets instead
• [x] https://github.com/getsentry/sentry-docs/pull/5759
  • Update the docs
• [ ] In v8, remove tracingOrigins (https://github.com/getsentry/sentry-javascript/issues/6230)

Node

• [x] https://github.com/getsentry/sentry-javascript/pull/6055
  • Add shouldCreateSpanForRequest option
  • Don't create a span and don't send headers in cases where shouldCreateSpanForReqeust returns false, regardless of tracePropagationTargets
• [x] https://github.com/getsentry/sentry-javascript/pull/6191
  • Move shouldCreateSpanForRequest and tracePropagationTargets to Http integration options
  • Deprecate shouldCreateSpanForRequest and tracePropagationTargets from client options
• [ ] Ensure new Http integration options are documented (https://github.com/getsentry/sentry-docs/pull/5810)
• [ ] In v8, remove shouldCreateSpanForRequest and tracePropagationTargets from client options (https://github.com/getsentry/sentry-javascript/issues/6230)

[lobsterkatie]

Default to always creating a span, regardless of tracingOrigins, unless shouldCreateSpanForRequest is defined and returns

Is this considered a non-breaking change since it fixes broken behaviour?

Hmmm, yeah, good question. I said, "We decided not to fix tracingOrigins because it'd be breaking" and then promptly added to the top of the todo list a change which does exactly that. My statement was "it's breaking" because that's what we'd discussed at the time, but upon second thought, I don't think it is, because a) as you say, it should have worked this way from the beginning, and so this is really just fixing broken behavior, and b) in general we don't consider it breaking when we start sending extra information we didn't used to send, which is really all this does. At that point, the effect of the v8 change would really just be to rename the by-that-point-correctly-behaving tracingOrigins option to tracePropagationTargets, rather than to create a new option with new behavior.

Lemme confirm with the team whether we want to change this now or wait until v8.

Rather than edit your long post, I've copied the tasks here and I'll create a PR for each and add the references.

Honestly, I think it's nice to be able to see right up top what's been done and hasn't, so feel free to continue to edit your comment above, but I will probably also copy the PR links into the main issue description. And thank you for pointing out that there should be an associated docs here. I'll add that, too!

[timfish]

I think it's nice to be able to see right up top what's been done and hasn't

Ohhh yes, much better at the top! I think since I'm not doing exactly one PR per checkbox I wanted to rejig the bullet points around but chickened out from modifying your carefully crafted summary. I've started this now so I shall endeavour to keep both updated 😂

[lobsterkatie]

Default to always creating a span, regardless of tracingOrigins, unless shouldCreateSpanForRequest is defined and returns

Is this considered a non-breaking change since it fixes broken behaviour?

Hmmm, yeah, good question. I said, "We decided not to fix tracingOrigins because it'd be breaking" and then promptly added to the top of the todo list a change which does exactly that. My statement was "it's breaking" because that's what we'd discussed at the time, but upon second thought, I don't think it is, because a) as you say, it should have worked this way from the beginning, and so this is really just fixing broken behavior, and b) in general we don't consider it breaking when we start sending extra information we didn't used to send, which is really all this does. At that point, the effect of the v8 change would really just be to rename the by-that-point-correctly-behaving tracingOrigins option to tracePropagationTargets, rather than to create a new option with new behavior.

Lemme confirm with the team whether we want to change this now or wait until v8.

Okay, just saw Abhi's comment on your first PR. I'm also on team "let's change it now," so I say we do it. 🚀

[lobsterkatie]

Welp. We should have moved those tests after all - it would have caught https://github.com/getsentry/sentry-javascript/issues/6077, which comes from the fact that we released a new version in between merging https://github.com/getsentry/sentry-javascript/pull/6039 and https://github.com/getsentry/sentry-javascript/pull/6041.

I'll pull the part that's the fix out so we can release it tomorrow and leave https://github.com/getsentry/sentry-javascript/pull/6041 as purely a "add the new option" PR.

[timfish]

For node.js:

• Ensure both options are documented
• In v8, consider moving both options into the Http integration

I was just about to document these additions.

How about:

• Add these settings to the Http integration now
• Deprecate them in BaseNodeOptions
• Prefer the integrations options but fallback to BaseNodeOptions so it not a breaking change
• Add docs for these as integration options

This would get the changes in motion and mean not doing the docs changes twice.

[lobsterkatie]

Sounds good, @timfish! Thanks!

[AbhiPrasad]

Considering https://github.com/getsentry/sentry-javascript/issues/6230 tracks the deprecations for v8, closing this as the docs/sdk work is done for now. Thanks for the :shipit: @timfish!