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

chore!: Make route path as transaction name in gin performance #675

Merged
merged 11 commits into from
Sep 4, 2023

Conversation

antonsacred
Copy link
Contributor

@antonsacred antonsacred commented Jul 27, 2023

This improvement should improve usability of sentry performance

This PR consists of two changes:

  1. Removed http method from transaction name (this is unnecessary because sentry performance has http method info already)
  2. Changed transaction name to context.FullPath() witch is more readable for endpoint owners

Changes are shown in screenshot. First one is before, second one is after
Screenshot 2023-07-27 at 2 54 17 PM

Copy link
Member

@cleptric cleptric left a comment

Choose a reason for hiding this comment

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

Can you confirm all routes will now be fully parameterized?

If yes, you can update sentry.WithTransactionSource(sentry.SourceURL) to sentry.WithTransactionSource(sentry.SourceRoute).

gin/sentrygin.go Outdated Show resolved Hide resolved
@antonsacred
Copy link
Contributor Author

Can you confirm all routes will now be fully parameterized?

If yes, you can update sentry.WithTransactionSource(sentry.SourceURL) to sentry.WithTransactionSource(sentry.SourceRoute).

thanks, done

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage is 100.00% of modified lines.

Files Changed Coverage
gin/sentrygin.go 100.00%

📢 Thoughts on this report? Let us know!.

@antonsacred
Copy link
Contributor Author

Any action required from my side?

@cleptric
Copy link
Member

@antonsacred No, all good. CI failed because our Windows tests started to be a bit flaky. We'll get this merged in the next few days.

gin/sentrygin.go Outdated
}

transaction := sentry.StartTransaction(ctx,
fmt.Sprintf("%s %s", c.Request.Method, c.Request.URL.Path),
fmt.Sprintf("%s %s", c.Request.Method, c.FullPath()),
Copy link
Member

Choose a reason for hiding this comment

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

For not found routes returns an empty string.

This causes issues for 404 routes, as the transaction name is now always something like GET .
We should check for the return of c.FullPath(). If it's empty, use c.Request.URL.Path as a fallback and set sentry.WithTransactionSource(sentry.SourceURL).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's cool that you found it! Done
3b50c09

Copy link
Contributor

Choose a reason for hiding this comment

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

@antonsacred I think what @cleptric suggests is more like, do the check and fallback here in handle directly (not in defer), and set TransactionSource depending on the value of c.FullPath().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonyo thanks for the take. I like your proposal. I have changed the code.

Copy link
Member

Choose a reason for hiding this comment

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

It would be great to test different paths, like /panic, /panic/:id etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c1edc11
This way I tried to show where request and router paths are in used

@antonsacred
Copy link
Contributor Author

@cleptric pls review again

@antonsacred antonsacred changed the title Sentry gin performance transaction improvement chore!: Make route path as transaction name in gin performance Sep 1, 2023
@cleptric cleptric merged commit cd0b874 into getsentry:master Sep 4, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants