-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add BrowserTracing integration #2723
feat: Add BrowserTracing integration #2723
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits and thoughts.
The biggest thing probably is that we should make the routingInstrumentationProcessors
async so people can work with a Promise here. We also do it on sentry.io so I think the use-case of this being async would be common.
Let's sync offline about this.
f0a2d4c
to
c8612c1
Compare
Ok I reworked a lot of this here. The aim of this PR is twofold:
This is done by creating a new integration. This integration has no static methods unlike
We do this by adding a new option for the /**
* Instrumentation that creates routing change transactions. By default creates
* pageload and navigation transactions.
*/
routingInstrumentation<T extends TransactionType>(
startTransaction: (context: TransactionContext) => T | undefined,
startTransactionOnPageLoad?: boolean,
startTransactionOnLocationChange?: boolean,
): void; This function is called with a I kept For example, this is how the react router integration would work for react router v3. // react router uses the history package internally.
import {History} from 'history';
export function reactRouterInstrumentation(history: History, routes: Routes[]) {
return function routingInstrumentation<T extends TransactionType>(
startTransaction: (context: TransactionContext) => T | undefined,
startTransactionOnPageLoad: boolean = true,
startTransactionOnLocationChange: boolean = true
): void {
let activeTransaction: T | undefined;
if (startTransactionOnPageLoad) {
activeTransaction = startTransaction({
// getNameFromRoutes is memoized :)
name: getNameFromRoutes(routes, window.location),
op: 'pageload',
});
}
if (startTransactionOnLocationChange) {
history.listen(location => {
if (location.action === 'PUSH') {
if (activeTransaction) {
activeTransaction.finish();
}
activeTransaction = startTransaction({
name: getNameFromRoutes(routes, window.location),
op: 'navigation',
});
}
});
}
};
}
// In another file
import {browserHistory} from 'react-router'
import { routes } from './routes'
Sentry.init({
dsn: SENTRY_DSN,
integrations: [
new Integrations.BrowserTracing({
routingInstrumentation: reactRouterInstrumentation(browserHistory, routes)
}),
],
tracesSampleRate,
});
|
df49b4e
to
27eb2d4
Compare
27eb2d4
to
6dd069c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a solid refactor + tests ❤️
* test: remove hub.startSpan test * feat(tracing): Add BrowserTracing integration and tests * fix: defaultRoutingInstrumentation * ref: Remove static methods * multiple before finishes * ref: Routing Instrumentation * remove tracing
* feat: Create IdleTransaction class (#2720) * feat: Add BrowserTracing integration (#2723) * feat: Add span creators to @sentry/tracing package (#2736) * ref: Convert React and Vue Tracing to use active transaction (#2741) * build: generate tracing bundles * fix: Remove circular dependency between span and transaction * ref: Add side effects true to tracing * build: Only include @sentry/browser for bundle * fix: Make sure vue and react are backwards compatible with @sentry/apm
Motivation
Following up my work in #2720, this PR adds the
BrowserTracing
integration.Right now it only creates pageload/navigation transactions, and in the next PR I will port over the rest of the span creators/editors (requests, metrics, errors and tab visibility).
Note: This stuff below is out of date, see the comment below: #2723 (comment)
Implementation
Pretty much the same as oldTracing
, on integration setup the pageload transaction starts, and on navigation the navigation transaction starts. I added some extra logic to prevent navigation transactions from cancelling pageload transactions as well.This PR addsroutingInstrumentationProcessors
, which are basically processors that can edit a transaction context before it gets created. I am not confident in how this API is structured, so I would appreciate any guidance on how to change it.My thought process was that a UI routing library could just add a processor to make sure that before a transaction is created, it has the correct tags and name. So React Router for example, would just callBrowserTracing.options. routingInstrumentationProcessors.push(blah)
This has the problem of declaration order, which I remember we discussed, but I'm not sure what the best way around this is. Thoughts? Maybe we should add logic to the integrations that any Tracing integration is always called first?Tests
This is verified to create transactions on latest Sentry master through manual testing. In addition I've unit tested all the functionality added.