[tracing] Separate span creation from header propagation

[lobsterkatie]

Note: This description has been rewritten to reflect changes since it was first created. The original version is included below for context.

Background:

Faced with an outgoing HTTP request, our SDKs have to make two decisions when tracing is enabled:

• Should a span be created to track this request? and
• Should sentry-trace and baggage headers be added to it?

Right now, we have three different options which affect the answers to those questions, and they behave differently in node and browser. The ultimate goal is to have consistent options across platforms, and - as much as possible - have each option only affect the things it says it affects.

Browser:

Currently, for browser, there are two options, tracingOrigins and shouldCreateSpanForRequest, and the behavior is as follows:

                                               | `tracingOrigins` match               | no `tracingOrigins` match            |
|----------------------------------------------|--------------------------------------|--------------------------------------|
| `shouldCreateSpanForRequest` returns `true`  | headers attached, span created       | no headers attached, no span created |
| `shouldCreateSpanForRequest` returns `false` | no headers attached, no span created | no headers attached, no span created |
| `shouldCreateSpanForRequest` is undefined    | headers attached, span created       | no headers attached, no span created |

The top right and bottom right cells are wrong - a span should be created, but it currently isn't. In other words, tracingOrigins is controlling span creation when it shouldn't. (It might appear that the middle left cell is also wrong - that headers should be attached in that case, because there's a tracingOrigins match - but headers without a parent span id make no sense, because their whole purpose is to help link transactions within a trace, so it is in fact correct.)

Because fixing the behavior of tracingOrigins would be a breaking change, and because its name has always been unnecessarily confusing (in tech speak, "origins" = "web domains," but in English, "origins" = "where things come from," which makes no sense in the context of outgoing requests), we decided to deprecate rather than fix it, in favor of introducing a new tracePropagationTargets option with the correct behavior.

So the changes needed on the browser side are:

• Default to always creating a span, regardless of tracingOrigins, unless shouldCreateSpanForRequest is defined and returns false (fix(tracing): Don't consider tracingOrigins when creating spans #6039 and fix(tracing): Fix tracingOrigins not applying #6079)
• Add tracePropagationTargets option with same default value as tracingOrigins (feat(tracing): Add tracePropagationTargets option to browser routing instrumentation #6080)
  • 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)
• Mark tracingOrigins as deprecated (ref(tracing): Deprecate tracingOrigins #6176)
  • Fix logging re: tracingOrigins to use tracePropagationTargets instead
• Document all of the above (Browser tracingOrigins is deprecated and replaced with tracePropagationTargets sentry-docs#5759)
• In v8, remove tracingOrigins (Remove deprecated tracing configuration options in v8 #6230)

Node:

Currently, for node, there is one option, tracePropagationTargets, and the behavior is as follows:

| `tracePropagationTargets` match      | no `tracePropagationTargets` match   |
|--------------------------------------|--------------------------------------|
| headers attached, span created       | no headers attached, span created    |

So the changes needed on the node side are:

• Add shouldCreateSpanForRequest option (feat(node): Add shouldCreateSpanForRequest option #6055)
  • Don't create a span and don't send headers in cases where shouldCreateSpanForReqeust returns false, regardless of tracePropagationTargets
• feat(node): Move tracing options to Http integration #6191
  • Move shouldCreateSpanForRequest and tracePropagationTargets to Http integration options
  • Deprecate shouldCreateSpanForRequest and tracePropagationTargets from client options
• Ensure new Http integration options are documented (Add new tracing options for node Http integration sentry-docs#5810)
• In v8, remove shouldCreateSpanForRequest and tracePropagationTargets from client options (Remove deprecated tracing configuration options in v8 #6230)

Ultimate Goal:

In the end, in both browser and node, the table should look like this:

                                               | `tracePropagationTargets` match      | no `tracePropagationTargets` match   |
|----------------------------------------------|--------------------------------------|--------------------------------------|
| `shouldCreateSpanForRequest` returns `true`  | headers attached, span created       | no headers attached, span created    |
| `shouldCreateSpanForRequest` returns `false` | no headers attached, no span created | no headers attached, no span created |
| `shouldCreateSpanForRequest` is undefined    | headers attached, span created       | no headers attached, span created    |

In browser, tracePropagationTargets should default to ['localhost', /^\//], whereas in node it should default to ['.*'].

While we're here, it might be nice to unify the implementation, both between options and between platforms, so that either

• both options have a default value on both platforms, and the undefined case therefore never needs to be handled at runtime, or
• neither option has a default value on either platform, and the undefined case is always handled (and the default value applied) at runtime.

Potentially related to getsentry/develop#611.

---

Original version of this issue, now outdated:

Click to expand

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. It turns out no other SDKs had these two concerns intertwined, and therefore the changes coming down the pike at that point fortunately didn't include trying to replicate this broken behavior. So this is just an issue for the JS SDK.

Current Situation:

Currently, the tracingOrigins 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 makes 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.

[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

• fix(tracing): Don't consider tracingOrigins when creating spans #6039 and fix(tracing): Fix tracingOrigins not applying #6079
  • Default to always creating a span, regardless of tracingOrigins, unless shouldCreateSpanForRequest is defined and returns false
• feat(tracing): Add tracePropagationTargets option to browser routing instrumentation #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)
• ref(tracing): Deprecate tracingOrigins #6176
  • Mark tracingOrigins as deprecated
  • Fix logging re: tracingOrigins to use tracePropagationTargets instead
• Browser tracingOrigins is deprecated and replaced with tracePropagationTargets sentry-docs#5759
  • Update the docs
• In v8, remove tracingOrigins (Remove deprecated tracing configuration options in v8 #6230)

Node

• feat(node): Add shouldCreateSpanForRequest option #6055
  • Add shouldCreateSpanForRequest option
  • Don't create a span and don't send headers in cases where shouldCreateSpanForReqeust returns false, regardless of tracePropagationTargets
• feat(node): Move tracing options to Http integration #6191
  • Move shouldCreateSpanForRequest and tracePropagationTargets to Http integration options
  • Deprecate shouldCreateSpanForRequest and tracePropagationTargets from client options
• Ensure new Http integration options are documented (Add new tracing options for node Http integration sentry-docs#5810)
• In v8, remove shouldCreateSpanForRequest and tracePropagationTargets from client options (Remove deprecated tracing configuration options in v8 #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 #6077, which comes from the fact that we released a new version in between merging #6039 and #6041.

I'll pull the part that's the fix out so we can release it tomorrow and leave #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 #6230 tracks the deprecations for v8, closing this as the docs/sdk work is done for now. Thanks for the @timfish!