fix(tracing): Pass tracePropagationTargets to instrumentOutgoingRequests

[Lms24]

This PR fixes a bug in our BrowserTracing integration which caused the new tracePropagationTargets option not to be passed to instrumentOutgoingRequests where it was needed to decide if our tracing headers should be attached to outgoing requests or not.

Because we never passed this value to the instrumentation function, custom-defined tracePropagationTargets values were not respected by the SDK and headers were attached to requests whose URLs matched the default targets or custom specified tracingOrigins.

With this PR, we also make a change how we internally handle the co-existance between the deprecated tracingOrigins and tracePropagationTargets options. We now simply overwrite the default tracePropagationTargets values with custom tracingOrigins (if available and no custom tracePropagationTargets were set). This enables us to internally only rely on tracePropagationTargets.
Note that we still have to keep setting tracingOrigins to browserTracing.options, as removing this field or changing the type would break users. This field however is not used internally anymore.

This PR also adds a bunch of unit and integration tests to make sure, tracePropagationTargets works as expected this time. Also, the tests check that tracingOrigins is still respected properly.

fixes #6077

[github-actions]

size-limit report 📦

Path	Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified)	19.52 KB (+0.01% 🔺)
@sentry/browser - ES5 CDN Bundle (minified)	60.39 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified)	18.18 KB (+0.03% 🔺)
@sentry/browser - ES6 CDN Bundle (minified)	53.74 KB (+0.04% 🔺)
@sentry/browser - Webpack (gzipped + minified)	19.91 KB (+0.02% 🔺)
@sentry/browser - Webpack (minified)	65.2 KB (+0.03% 🔺)
@sentry/react - Webpack (gzipped + minified)	19.94 KB (+0.01% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified)	45.95 KB (+0.04% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified)	26.38 KB (+0.09% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified)	24.79 KB (+0.1% 🔺)

[AbhiPrasad]

Won't this be breaking instrumentOutgoingRequests (we publicly export this function)

mistake

[Lms24]

Won't this be breaking instrumentOutgoingRequests (we publicly export this function)

Hmm didn't know that we export this publicly. But would it really be breaking? We don't change the signature at all, we just don't take tracingOrigins anymore from the passed _options parameter.

EDIT: Ok, sure if people still pass tracingOrigins in there, it'd not be respected anymore (i.e. behaviour-breaking). I can add the same merge snippet we have in setupOnce also into this function until v8.

Out of curiosity, do you know why we expose this function? I couldn't find any usage of it in our SDKs

[AbhiPrasad]

we just don't take tracingOrigins anymore from the passed _options parameter.

So if someone passes in tracingOrigins it is no longer being used, so that would break people. It's probably rarely used though, so I don't mind breaking it, let's ship this then.

[ghost]

Where does this option go? The docs don't give any context.

[AbhiPrasad]

tracePropagationTargets is an option on the BrowserTracing integration, same as tracingOrigins was.

https://docs.sentry.io/platforms/javascript/performance/instrumentation/automatic-instrumentation/#tracepropagationtargets

Sentry.init({
  // ...
  integrations: [
    new BrowserTracing({
      tracePropagationTargets: ["api.example.com"],
    }),
  ],
});

[ghost]

Thanks! The top 2 documentation results when you search for the tracePropagationTargets flag don't show that example.

• https://develop.sentry.dev/sdk/performance/
• https://docs.sentry.io/platforms/node/configuration/options/#trace-propagation-targets

Also, I got screwed because I was passing in tracingOrigins. However... my big question is why setting traceFetch and traceXHR to false didn't work? According to those docs, it should stop Sentry from modifying my requests altogether...

Should I submit another issue to fix those docs?

[ghost]

This really, really screwed up our Beamer integration - a task which should have literally taken 1 minute ended up taking hours to figure out why their requests were being rejected.

[AbhiPrasad]

Sorry to hear the troubles - thanks for sharing your feedback, we'll make sure to do better here.

Regarding the docs you linked.

• https://develop.sentry.dev/sdk/performance/: These are our Sentry internal developer docs.
• https://docs.sentry.io/platforms/node/configuration/options/#trace-propagation-targets: These are the Node platform docs. You can identify this by the platforms/node in the URL.

It's important to note, tracingOrigins was deprecated, and not out right removed. It should still work as before. If it doesn't this is a bug on our side, and it would be great if you could open a new issue.

Setting traceFetch and traceXHR to false should also work here - we can investigate that in the issue you open as well.

[ghost]

I was still on 7.7 and still passing tracingOrigins while trying to set traceFetch/traceXHR to false so I'm going to assume that's what was happening. Thanks again!

[ghost]

This tracePropagationTargets is still not working on 7.31.1. Here is my code:

import {
  init as initSentry,
  reactRouterV5Instrumentation,
} from "@sentry/react";
import { BrowserTracing } from "@sentry/tracing";

initSentry({
    dsn: REACT_APP_SENTRY_DSN,
    integrations: [
      new BrowserTracing({
        tracePropagationTargets: ["api.mydomain.com", "www.mydomain.com"],
        routingInstrumentation: reactRouterV5Instrumentation(history),
      }),
    ],
    // Set tracesSampleRate to 1.0 to capture 100% of transactions.
    // We recommend adjusting this value in production.
    tracesSampleRate: 1.0,
  });

And I still get error:

Access to XMLHttpRequest at 'https://backend.getbeamer.com/initialize?product=XXXX&domain=www.mydomain.com&language=EN' from origin 'https://www.mydomain.com' has been blocked by CORS policy: Request header field baggage is not allowed by Access-Control-Allow-Headers in preflight response.

I can see in the devtools that Sentry is still adding the baggage header...

baggage: sentry-public_key=xxxxyyy,sentry-trace_id=12341234,sentry-sample_rate=1

[AbhiPrasad]

As per the docs on tracePropagationTargets

The tracePropagationTargets option matches the entire request URL, not just the domain. Using stricter regex to match certain parts of the URL ensures that requests don't unnecessarily have additional headers attached.

In this case, https://backend.getbeamer.com/initialize?product=XXXX&domain=www.mydomain.com&language=EN url has domain=www.mydomain.com in it, so it gets matched by tracePropagationTargets. I would use a regex to avoid this.

[ghost]

Thanks so much for your help @AbhiPrasad

Looking at the docs' example while in the middle of triaging a frustrating issue where it says api.domain.com was misleading. The example makes you think it will just match the domain and as a matter of fact, I don't know why api.domain.com is useful at all as an example or an implementation if it's just going to match the whole request url anyway.

In other words - who would ever specify just api.domain.com in their config? I would change that example to put https://api.domain.com or use a regex example because I doubt I'll be the last person to just blindly look at your example and assume that this is the case.

Honestly, I was surprised after using Sentry for years that it's patching every request to begin with...

I'm assuming that something like this will work and I'm going to test that in a few moments. I understand the full extent of the issue now and I just wanted to share my experience dealing with it as I sit here waiting for the CI/CD to deploy....

       tracePropagationTargets: [
          "https://api.mydomain.com",
          "https://www.mydomain.com",
        ],

[ghost]

It finally worked. I'm going to go rethink my life now. Thanks again!!

[ghost]

I also asked to have this sent to Beamer so they can warn anyone using Sentry that it will be a real PITA to figure out if you've never dealt with these options before.

[AbhiPrasad]

The example makes you think it will just match the domain and as a matter of fact, I don't know why api.domain.com is useful at all as an example or an implementation if it's just going to match the whole request url anyway.

Yup I understand why this can be confusing, we are going to take another look at evaluating this.

Honestly, I was surprised after using Sentry for years that it's patching every request to begin with

For performance monitoring we need to patch every request (that you allow in tracePropagationTargets) for our distributed tracing functionality. This only happens when you are using performance monitoring.

We do monkeypatch fetch/xhr for errors - but that doesn't mutate the outgoing headers.

Thanks for being patient with us - apologies for the trouble caused!