Skip to content

fix(apm): Set the transaction name for JavaScript transactions before they are flushed #18822

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

Merged
merged 6 commits into from
May 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/sentry/static/sentry/app/bootstrap.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import ConfigStore from 'app/stores/configStore';
import Main from 'app/main';
import ajaxCsrfSetup from 'app/utils/ajaxCsrfSetup';
import plugins from 'app/plugins';
import routes from 'app/routes';
import {normalizeTransactionName} from 'app/utils/apm';

function getSentryIntegrations(hasReplays: boolean = false) {
const integrations = [
Expand Down Expand Up @@ -74,11 +76,16 @@ const tracesSampleRate = config ? config.apmSampling : 0;
const hasReplays =
window.__SENTRY__USER && window.__SENTRY__USER.isStaff && !!process.env.DISABLE_RR_WEB;

const appRoutes = Router.createRoutes(routes());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, I cannot move this to src/sentry/static/sentry/app/utils/apm.tsx

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you move it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The app doesn't load.


Sentry.init({
...window.__SENTRY__OPTIONS,
integrations: getSentryIntegrations(hasReplays),
tracesSampleRate,
_experiments: {useEnvelope: true},
async beforeSend(event) {
return normalizeTransactionName(appRoutes, event);
},
});

if (window.__SENTRY__USER) {
Expand Down
65 changes: 65 additions & 0 deletions src/sentry/static/sentry/app/utils/apm.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
import * as Sentry from '@sentry/browser';
import * as Router from 'react-router';
import {createMemoryHistory} from 'history';
import set from 'lodash/set';

import getRouteStringFromRoutes from 'app/utils/getRouteStringFromRoutes';

const createLocation = createMemoryHistory().createLocation;

/**
* Sets the transaction name
Expand All @@ -9,3 +16,61 @@ export function setTransactionName(name: string) {
scope.setTag('ui.route', name);
});
}

export async function normalizeTransactionName(
appRoutes: Router.PlainRoute[],
event: Sentry.Event
): Promise<Sentry.Event> {
if (event.type !== 'transaction') {
return event;
}

// For JavaScript transactions, translate the transaction name if it exists and doesn't start with /
// using the app's react-router routes. If the transaction name doesn't exist, use the window.location.pathname
// as the fallback.

let prevTransactionName = event.transaction;

if (typeof prevTransactionName === 'string') {
if (prevTransactionName.startsWith('/')) {
return event;
}

set(event, ['tags', 'transaction.rename.source'], 'existing transaction name');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@billyvg nah I don't think so. I think, semantically, tags that begin transaction.rename should only apply to events that have undergone a transaction rename process.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, we shouldn't be renaming transactions that begin with /. Since these transactions are what we'd expect. They've already been renamed at the React app level from within the lifecycle method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, the word "existing" kind of threw me off (I was thinking existing mean it has a correct transaction name).

} else {
set(event, ['tags', 'transaction.rename.source'], 'window.location.pathname');

prevTransactionName = window.location.pathname;
}

const transactionName: string | undefined = await new Promise(function(resolve) {
Router.match(
{
routes: appRoutes,
location: createLocation(prevTransactionName),
},
(error, _redirectLocation, renderProps) => {
if (error) {
set(event, ['tags', 'transaction.rename.react-router-match'], 'error');
return resolve(undefined);
}

set(event, ['tags', 'transaction.rename.react-router-match'], 'success');

const routePath = getRouteStringFromRoutes(renderProps.routes ?? []);
return resolve(routePath);
}
);
});

if (typeof transactionName === 'string' && transactionName.length) {
event.transaction = transactionName;

set(event, ['tags', 'transaction.rename.before'], prevTransactionName);
set(event, ['tags', 'transaction.rename.after'], transactionName);

set(event, ['tags', 'ui.route'], transactionName);
}

return event;
}