feat(tracing): Expose new `browserTracingIntegration` by mydea · Pull Request #10351 · getsentry/sentry-javascript

mydea · 2024-01-25T19:33:15Z

Extracted this out of #10327.

This PR:

Introduces a new browserTracingIntegration()
Does NOT deprecate BrowserTracing yet, as custom implementations in Angular, Next, Sveltekit have not been migrated over yet, which would be weird. We can deprecate it once we moved these over.
Makes sure that custom implementations in Next & Sveltekit are "fixed" automatically
Uses a slim fork for the CDN bundles, to avoid shipping multiple implementations in there.
- This means that in the CDN bundles, you can already use the new syntax, but you cannot pass a custom routing instrumentation anymore, and you also don't have the utility functions for it yet. I think this is the best tradeoff for now, and it's probably not a super common case to have custom routing instrumentation when using the Loader/CDN bundles (and if you do, you have to stick to new BrowserTracing() until v8).

I copied the browser integration tests we have, which all pass!

mydea · 2024-01-25T19:34:46Z

packages/browser/src/index.ts

  defaultRequestInstrumentationOptions,
  instrumentOutgoingRequests,
+  browserTracingIntegration,
+  startBrowserTracingNavigationSpan,


These two APIs would be used by custom instrumentation, e.g. Angular, Ember, Next etc.

mydea · 2024-01-25T19:35:47Z

packages/nextjs/src/client/index.ts

+  }
+
+  // If `browserTracingIntegration()` was added, we need to force-convert it to our custom one
+  if (isNewBrowserTracingIntegration(browserTracing)) {


This is all pretty hacky 😬 but the best I could come up with! It should work well enough for 99% of cases.

packages/tracing-internal/src/browser/browserTracingIntegration.ts

mydea · 2024-01-25T19:41:17Z

packages/tracing-internal/src/browser/browserTracingIntegration.ts

+    },
+    // TODO v8: Remove this again
+    // This is private API that we use to fix converted BrowserTracing integrations in Next.js & SvelteKit
+    options,


Could not think of a better way to allow us to solve this 😬 I'd say it's OK for now, we can remove it in v8.

We need to refactor how options works in general with BrowserTracing, there's some duplication around setting defaults that I think we can improve.

github-actions · 2024-01-25T19:47:41Z

size-limit report 📦

Path	Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped)	77.89 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped)	69.08 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) - Webpack (gzipped)	72.97 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped)	62.72 KB (+0.02% 🔺)
@sentry/browser (incl. Tracing) - Webpack (gzipped)	33.12 KB (+0.05% 🔺)
@sentry/browser (incl. Feedback) - Webpack (gzipped)	31.28 KB (0%)
@sentry/browser (incl. sendFeedback) - Webpack (gzipped)	31.29 KB (0%)
@sentry/browser - Webpack (gzipped)	22.54 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped)	75.71 KB (+0.14% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped)	67.26 KB (+0.16% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped)	33.11 KB (+0.32% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped)	24.47 KB (+0.03% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed)	211.89 KB (+0.2% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed)	99.91 KB (+0.43% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed)	73.23 KB (+0.08% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped)	36.2 KB (+0.25% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped)	69.49 KB (+0.02% 🔺)
@sentry/react - Webpack (gzipped)	22.58 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped)	86.19 KB (+0.05% 🔺)
@sentry/nextjs Client - Webpack (gzipped)	50.49 KB (+0.1% 🔺)
@sentry-internal/feedback - Webpack (gzipped)	17.21 KB (0%)
@sentry/browser (incl. browserTracingIntegration) - Webpack (gzipped)	33.02 KB (added)

AbhiPrasad

A little afraid I missed reviewing an export somewhere, but otherwise looks good!

mydea · 2024-01-25T21:48:29Z

So I added two further utilities:

disableDefaultBrowserTracingNavigationSpan()
disableDefaultBrowserTracingPageLoadSpan()

These can be used by custom instrumentation to ensure we don't trigger the default spans.

Lms24 · 2024-01-26T08:57:14Z

packages/tracing-internal/src/browser/browserTracingIntegration.ts

+    // eslint-disable-next-line deprecation/deprecation
+    const sourceFromData = context.data && context.data[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE];
+    // eslint-disable-next-line deprecation/deprecation
+    const sourceFromMetadata = finalContext.metadata && finalContext.metadata.source;


m: Shouldn't we additionally look for the source in context.attributes[SEMANTIC_ATTRIBUTES_SENTRY_SOURCE]?

I just directly copied this over for now, metadata is a getter that merges that attribute in, so it should still work normally :) in v8 we adjust this then!

ah wait nevermind, my comment is incorrect, I think you're right, let's also check attributes 👍

added a check for this, also in the original browser tracing integration!

Lms24

Looks good to me!

Also deprecate the routing Instrumentation. This is WIP on top of #10351, to show how that would work. No idea how to get the tests working for this, though...

mydea requested review from AbhiPrasad, Lms24 and lforst January 25, 2024 19:33

mydea self-assigned this Jan 25, 2024

mydea commented Jan 25, 2024

View reviewed changes

packages/tracing-internal/src/browser/browserTracingIntegration.ts Show resolved Hide resolved

mydea commented Jan 25, 2024

View reviewed changes

AbhiPrasad approved these changes Jan 25, 2024

View reviewed changes

mydea mentioned this pull request Jan 25, 2024

feat(angular): Export custom browserTracingIntegration() #10353

Merged

mydea added 4 commits January 25, 2024 16:44

feat(tracing): Expose new browserTracingIntegration

c1cae1a

better comments

a0a79fa

fix linting

e4fc705

add method to disable defaults

af7fc5c

mydea force-pushed the fn/newBrowserTracingIntegrationOnly branch from ed19b05 to af7fc5c Compare January 25, 2024 21:48

Lms24 reviewed Jan 26, 2024

View reviewed changes

Lms24 approved these changes Jan 26, 2024

View reviewed changes

fix source handling

2f12861

mydea merged commit adccbe6 into develop Jan 26, 2024

mydea deleted the fn/newBrowserTracingIntegrationOnly branch January 26, 2024 15:54

mydea mentioned this pull request Jan 29, 2024

[v8] Replace custom BrowserTracing routing instrumentations #10387

Closed

Uh oh!

Conversation

mydea commented Jan 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

AbhiPrasad left a comment

Choose a reason for hiding this comment

Uh oh!

mydea commented Jan 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lms24 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Jan 25, 2024 •

edited

Loading