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

ref: Give http transactions better names #687

Merged
merged 1 commit into from
Mar 2, 2022
Merged

ref: Give http transactions better names #687

merged 1 commit into from
Mar 2, 2022

Conversation

Swatinem
Copy link
Member

@Swatinem Swatinem commented Feb 22, 2022

This solves the problem of having a ton of unique GET /requests/UUID performance monitoring transactions.

Also updates the Sentry SDK for newly introduced API.

#skip-changelog

@jan-auer
Copy link
Member

@Swatinem where was the original transaction name assigned, is that done by an integration?

@Swatinem
Copy link
Member Author

Yes, the tower-http layer does that. It only has access to the URL, not the matcher itself.

@jan-auer
Copy link
Member

It only has access to the URL, not the matcher itself.

Thanks, that's interesting. We're seeing similar issues in our React integration, so maybe this will require a broader solution after all in the long-term. cc @jjbayer

@Swatinem In the interim, it would be valuable to document this workaround. Effectively everyone using the tower integration has to check and work around this to get proper transactions.

@Swatinem Swatinem marked this pull request as ready for review March 2, 2022 10:57
@Swatinem Swatinem requested a review from a team March 2, 2022 10:57
@@ -299,7 +299,6 @@ impl CfiCacheActor {
#[tracing::instrument(skip_all)]
fn write_cficache(path: &Path, object_handle: &ObjectHandle) -> Result<(), CfiCacheError> {
configure_scope(|scope| {
scope.set_transaction(Some("compute_cficache"));
Copy link
Contributor

Choose a reason for hiding this comment

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

in theory it seems to me this should have been some kind of nested transaction inside the main request one? does the product handle nested transactions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely. We only use them for lazy recomputations though, normal cache computation is a span instead, as per the instrument above.

Copy link
Contributor

Choose a reason for hiding this comment

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

so i'm curious then why you decided to remove these transactions?

Copy link
Member Author

Choose a reason for hiding this comment

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

because they would overwrite the symbolicate_stacktraces name.

@Swatinem Swatinem merged commit 71b494d into master Mar 2, 2022
@Swatinem Swatinem deleted the ref/trx-name branch March 2, 2022 13:57
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.

None yet

3 participants