Skip to content

fix(js-sdk): Move tracePropagationTargets to top level config - #51900

Merged
AbhiPrasad merged 1 commit into
masterfrom
abhi-trace-propagation-target
Jun 29, 2023
Merged

fix(js-sdk): Move tracePropagationTargets to top level config#51900
AbhiPrasad merged 1 commit into
masterfrom
abhi-trace-propagation-target

Conversation

@AbhiPrasad

Copy link
Copy Markdown
Contributor

In #51797 we bumped the JS SDK to 7.57.0. Changelog here: https://github.com/getsentry/sentry-javascript/releases/tag/7.57.0

Notable in this release was adding the ability to set tracePropagationTargets as a top level option to Sentry.init. Previously tracePropagationTargets was a nested option inside the BrowserTracing integration.

// old way
Sentry.init({
  integrations: [
   Sentry.BrowserTracing({
      tracePropagationTargets: ['foo', 'bar'],
    }),
  ],
});

// new with `7.57.0`
Sentry.init({
  integrations: [new Sentry.BrowserTracing()],
  // controls what requests trace headers attached - required because cors 😭
  tracePropagationTargets: ['foo', 'bar']
});

This change is done in a backwards compat manner though, so that if you defined tracePropagationTargets in BrowserTracing, it is considered valid. What's notable is that we made the decision to say that defining a top level tracePropagationTargets (in Sentry.init) would override the tracePropagationTargets set in BrowserTracing options - this minimized the bundle size of the backwards compat change.

Unfortunately, our sdk init code in the sentry frontend was unintentionally setting tracePropagationTargets in the top level, via the spread operator.

sentryConfig as {
  allowUrls: string[];
  dsn: string;
  release: string;
  tracePropagationTargets: string[];
  profilesSampleRate?: number;
};

Sentry.init({
    // This defines tracePropagationTargets on the init options object
    ...sentryConfig,
// ...

Previously this did nothing, but with the bump to 7.57.0, we started to use this top level tracePropagationTargets instead of the one in BrowserTracing. This means we removed the /^\// target in ['localhost', /^\//, ...extraTracePropagationTargets], which meant we stopped attaching headers to our outgoing api requests.

This PR fixes this predicament by removing the usage of BrowserTracing tracePropagationTargets in favour of using the top level option.

@AbhiPrasad
AbhiPrasad requested review from JonasBa, k-fish and wmak June 29, 2023 18:48
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jun 29, 2023
@AbhiPrasad

Copy link
Copy Markdown
Contributor Author

Validated this in dev-ui and running sentry locally, but would appreciate if someone also pulled and tested to double check.

@AbhiPrasad
AbhiPrasad enabled auto-merge (squash) June 29, 2023 19:01
@AbhiPrasad
AbhiPrasad merged commit 8baef7c into master Jun 29, 2023
@AbhiPrasad
AbhiPrasad deleted the abhi-trace-propagation-target branch June 29, 2023 19:07
AbhiPrasad added a commit that referenced this pull request Jun 29, 2023
)

This is a regression test to confirm that
#51900 fixed inc-433. See
https://getsentry.atlassian.net/browse/INC-433 for more details about
the incident.

It's purposefully a bare bones test - there's too many side effects with
the sentry lib to unit test properly, so I opted for a very specific
test that examines if we are setting `tracePropagationTargets` correctly
- as messing this up ruins our backend observability.
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants