Improve tracing #580
Merged
Improve tracing #580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cleptric
reviewed
Oct 6, 2022
There was a problem hiding this comment.
The main reason for this change is to properly support the use case where the Laravel SDK is head of trace, setting the baggage in the meta tag. Having the transaction name figured out before the view is rendered is something we have to do.
Setting unified transaction names based on the route version sounds like a good idea.
Worth checking what happens in getsentry/sentry-python#1661 (comment). This will go out as a new major version anyway, but maybe we get some insights onto how many dashboards could be affected.
As mentioned in #562, I would wait for changes to how we instrument HEAD/OPTIONS requests for now.
A way to enable instrumentation on 404s sounds good to me.
# Conflicts: # CHANGELOG.md # README.md # src/Sentry/Laravel/Tracing/EventHandler.php # src/Sentry/Laravel/Tracing/Middleware.php # test/Sentry/IntegrationTest.php
cleptric
approved these changes
Oct 10, 2022
* Remove extracting route name or controller for transaction names * CS * Add changelog entry
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We used to set the transaction name as late as possible to make sure the routing information is available to us. However this doesn't work well with generating the new
baggageso we now do it as early as Laravel allows us instead.Few things I like to improve here still:
RouteMatchedevent for setting the transaction nameHEADrequestsDiscussion points:
some.route) and controller class (SomeController@someAction) as options for transaction names and only keep the "route" version of the URL (some/{route})?HEADrequest and hard-404 tracing?