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

dashed · 2020-05-14T11:20:18Z

For JavaScript transactions, translate the transaction name if it exists and doesn't start with /
using the app's react-router routes; the expected transaction name would be the URL, which is set by the JS SDK.

If the transaction name doesn't exist, use the window.location.pathname as the fallback.

The transaction name is translated based on the app react-router routes using the react-router utility functions.

React router utility functions used https://github.com/ReactTraining/react-router/blob/v3.2.0/docs/API.md#utilities

This should deal with the race condition between setting the transaction name during the React render reconciliation and when transactions are flushed.

TODO

update ui.route tag

src/sentry/static/sentry/app/bootstrap.tsx

HazAT

Can we remove the code from app/utils/apm.tsx that sets the transaction?

src/sentry/static/sentry/app/bootstrap.tsx

HazAT · 2020-05-14T16:34:28Z

src/sentry/static/sentry/app/bootstrap.tsx

+            location: createLocation(prevTransactionName),
+          },
+          (error, _redirectLocation, renderProps) => {
+            if (error) {


Do we know the cases when this might fail?
Because in those cases we would still get the full url.
I just wanted to highlight this.

Unfortunately we may not know.

We need to at least handle this otherwise the transaction name will be overridden by a worst choice of a transaction name; in this case, a blank string.

benvinegar · 2020-05-14T18:11:43Z

How do we document this in our SDKs, framework docs, etc. so that other people can easily follow this example? cc @HazAT

billyvg · 2020-05-14T18:29:42Z

@HazAT I've noticed that we do not call the set transaction name on DidMount - only on DidUpdate, wonder if this would solve some of the problems we're having.

@dashed and I were talking about removing utils/apm but there could be cases where window.location changes before the transaction event is sent, in which case the route would not be accurate.

dashed · 2020-05-14T20:58:06Z

Going to put this PR on pause and try #18836 for a week or so.

lobsterkatie · 2020-05-14T22:45:38Z

@benvinegar Revisiting the docs explaining how to APM in Python/JS is my next project. We currently have an example of instrumenting a react component which I'd like to flesh out and clean up a bit. Will definitely be looking for feedback on that section once I have a draft, since I'm still fairly new to react.

billyvg · 2020-05-19T23:40:21Z

src/sentry/static/sentry/app/utils/apm.tsx

+      event.transaction = transactionName;
+
+      if (event.tags) {
+        event.tags['ui.route'] = transactionName;


Should we tag this event to identify how often this happens?

@billyvg do you mean analytics?

@dashed Yeah, might be nice to have it as a tag in the event

@billyvg is this something you had in mind? 58bb0bf

@dashed Yeah, you may be able to simplify to prev !== transactionName if you don't need the prev transaction name (since you already have transaction name).

@billyvg I'm unsure what you meant. prevTransactionName !== transactionName should be guaranteed to be true in this code path.

Ah, then the previous tag should be enough, and if you wanted to know the number of transactions that required normalization you can query for something like !transaction.rename.before:''?

I think has:transaction.rename.source might be the search condition. Since this gets set knowing that normalization is required. And regardless of whether or not the normalization process was successful.

dashed · 2020-05-20T00:35:19Z

src/sentry/static/sentry/app/bootstrap.tsx

@@ -70,10 +72,15 @@ 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());


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

What happens if you move it?

The app doesn't load.

billyvg · 2020-05-20T01:13:55Z

src/sentry/static/sentry/app/utils/apm.tsx

+      return event;
+    }
+
+    set(event, ['tags', 'transaction.rename.source'], 'existing transaction name');


Do we want this before https://github.com/getsentry/sentry/pull/18822/files#diff-f8570bef96092fcfcfc16de1804f940dR36?

@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.

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.

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

billyvg

Lgtm

… they are flushed Set the transaction name for JavaScript transactions based on latest window.location object just before the transaction is flushed. The window.location object is translated based on the app react-router routes using the react-router utility functions.

addressed

dashed requested review from billyvg, HazAT and a team May 14, 2020 11:20

dashed self-assigned this May 14, 2020

dashed force-pushed the apm-fix-transaction-name branch from 73048dc to 67c4264 Compare May 14, 2020 12:00

vercel bot deployed to Preview May 14, 2020 12:00 View deployment

vercel bot deployed to Preview May 14, 2020 14:16 View deployment

markstory reviewed May 14, 2020

View reviewed changes

src/sentry/static/sentry/app/bootstrap.tsx Show resolved Hide resolved

HazAT previously requested changes May 14, 2020

View reviewed changes

vercel bot deployed to Preview May 14, 2020 17:14 View deployment

dashed mentioned this pull request May 14, 2020

fix(apm): Set transaction name on componentDidMount() #18836

Merged

dashed force-pushed the apm-fix-transaction-name branch from 3a50682 to 515b1b2 Compare May 19, 2020 23:09

vercel bot deployed to Preview May 19, 2020 23:09 View deployment

vercel bot deployed to Preview May 19, 2020 23:23 View deployment

dashed requested review from markstory, HazAT and a team May 19, 2020 23:35

billyvg reviewed May 19, 2020

View reviewed changes

dashed force-pushed the apm-fix-transaction-name branch from 7a5ba50 to 3149baf Compare May 19, 2020 23:56

vercel bot deployed to Preview May 19, 2020 23:57 View deployment

vercel bot deployed to Preview May 20, 2020 00:34 View deployment

dashed commented May 20, 2020

View reviewed changes

dashed requested a review from billyvg May 20, 2020 00:35

billyvg reviewed May 20, 2020

View reviewed changes

billyvg approved these changes May 20, 2020

View reviewed changes

dashed added 6 commits May 20, 2020 06:16

translate transaction name if it exists and doesn't start with /

579cacc

docs

bb11e49

consolidate code

542bce1

fix

194338d

add tags to track the transaction rename

5249c9b

dashed force-pushed the apm-fix-transaction-name branch from 58bb0bf to 5249c9b Compare May 20, 2020 10:16

vercel bot deployed to Preview May 20, 2020 10:16 View deployment

dashed merged commit 4065210 into master May 20, 2020

dashed deleted the apm-fix-transaction-name branch May 20, 2020 19:37

This was referenced May 21, 2020

fix(apm): Adjust transaction name normalization #18968

Merged

feat(am): Introduce startTransaction getsentry/sentry-javascript#2600

Merged

github-actions bot locked and limited conversation to collaborators Dec 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dashed commented May 14, 2020 •

edited

Loading

HazAT left a comment

HazAT May 14, 2020

dashed May 19, 2020

benvinegar commented May 14, 2020

billyvg commented May 14, 2020

dashed commented May 14, 2020

lobsterkatie commented May 14, 2020

billyvg May 19, 2020

dashed May 19, 2020

billyvg May 19, 2020

dashed May 20, 2020

billyvg May 20, 2020

dashed May 20, 2020

billyvg May 20, 2020

dashed May 20, 2020

dashed May 20, 2020

billyvg May 20, 2020

dashed May 20, 2020

billyvg May 20, 2020

dashed May 20, 2020

dashed May 20, 2020

billyvg May 20, 2020

billyvg left a comment

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

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

Conversation

dashed commented May 14, 2020 • edited Loading

TODO

HazAT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvinegar commented May 14, 2020

billyvg commented May 14, 2020

dashed commented May 14, 2020

lobsterkatie commented May 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billyvg left a comment

Choose a reason for hiding this comment

dashed commented May 14, 2020 •

edited

Loading