Skip to content
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: Allow routeNameExtractor to set transaction names #1005

Merged
merged 3 commits into from
Sep 15, 2022
Merged

feat: Allow routeNameExtractor to set transaction names #1005

merged 3 commits into from
Sep 15, 2022

Conversation

JaspervanRiet
Copy link
Contributor

@JaspervanRiet JaspervanRiet commented Sep 13, 2022

📜 Description

This PR adds the functionality to have the routeNameExtractor override the route names logged in Sentry, which I personally think makes more sense. It does not make sense to me that this extractor would only affect breadcrumbs, which is the current state, see Motivation.

Note that this is currently technically a breaking change. I'm open to changing the PR to avoid that, but I'd say the decision on how to proceed there is best left to the maintainers of this project. :)

💡 Motivation and Context

Setting the routeNameExtractor of the SentryNavigatorObserver currently only affects breadcrumbs. For our app, this does not work. We use named routes with IDs to navigate to our pages, e.g. /movies/1500 to navigate to the detail page of a movie. We want to measure the performance of this whole group of pages instead of the unique instance. We thought we would be able to use the extractor to do this, but sadly not.

As for why I think this makes more sense: routeNameExtractor to me says that this functionality will allow you to mutate the route. Intuitively, that means this data will be used throughout the SentryNavigatorObserver; nothing about this signifies that this would only affect breadcrumbs. I see no discussion about this in the introducing PR (#684), so hard to say if this was thought about.

💚 How did you test it?

I used the current tests available for the observer, and added an extra test specific to this functionality since one was missing for the routeNameExtractor.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@marandaneto
Copy link
Contributor

@JaspervanRiet thanks for doing this.
I agree with the approach, makes sense.
I'd generify the code to call the _routeNameExtractor only once tho, right now it's being called multiple times in multiple places, better to just pass the routeName as a param to the methods.
It's a breaking change but I believe this was an oversight and it should be fixed, right now route names in crumbs and transactions might not be aligned if using the extractor.
What do you think @ueman ?

@ueman
Copy link
Collaborator

ueman commented Sep 14, 2022

Yes, sounds reasonable to me, too.

@JaspervanRiet JaspervanRiet requested review from marandaneto and removed request for brustolin September 14, 2022 08:37
@marandaneto
Copy link
Contributor

@JaspervanRiet please add a changelog entry and we are ready to go :)

@JaspervanRiet
Copy link
Contributor Author

@marandaneto Should be done!

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2022

Codecov Report

Base: 89.66% // Head: 89.66% // No change to project coverage 👍

Coverage data is based on head (3f0882f) compared to base (7987216).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1005   +/-   ##
=======================================
  Coverage   89.66%   89.66%           
=======================================
  Files         106      106           
  Lines        3329     3329           
=======================================
  Hits         2985     2985           
  Misses        344      344           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@marandaneto marandaneto merged commit 4897f58 into getsentry:main Sep 15, 2022
@JaspervanRiet JaspervanRiet deleted the fix-sentry-navigator-not-using-extractor branch September 15, 2022 07:16
@JaspervanRiet
Copy link
Contributor Author

Thanks for quick handling of this PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants