-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add Performance Monitoring (transactions) support #59
Conversation
Hi! Thanks for the contribution. Sorry, the notification about this PR got lost among others. This expands the scope of Would you be interested in taking over the maintainership of this crate? I can still continue as a co-maintainer and help with reviews, if you want. |
fn request_to_transaction_name(request: &Request) -> String { | ||
let method = request.method(); | ||
let path = request.uri().path(); | ||
format!("{method} {path}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine users may want to customize the transaction name. So possibly as a follow-up, it may make sense to provide a "request to transaction name" hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how it would looks like but nice idea. Some sentry sdk propose out of the box for transactions names to be returned as url or endpoint/function name. A custom hook could accomplish the same thing.
I agree, that's worth trying out and fixing. But feel free to leave it to a follow-up PR. |
Also README needs updating, this is no longer true:
|
default is no transaction enabled
Hi, thanks for taking the time to review the PR.
I tried to list the transaction's fields. Let me know what you think of the changes. |
@intgr I have added headers as well as I have the need for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for so many rounds of back-and-forth, I don't feel very competent reviewing this stuff yet. :)
Once everything is good for you, I should probably squash the commits in a single one.
No need, I'll use GitHub's "squash and merge"
Co-authored-by: Marti Raudsepp <marti@juffo.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good! Sorry again for procrastinating on this.
Great ! Don't forget to release it on crates.io, so I can switch back to using the crate. Cheers! |
Hello, this PR add support for performance/transaction.
It requires
sentry_transaction_sample_rate
to be set with a value above 0, else transactions are not enabled.A downside is that raw URI are used and so for a common route (with an id as parameter for example), transaction will not be aggregated. To fix it, we need to set the transaction name in the
on_response
callback as the routing is not yet done inon_request
. But I didn't find a way to set a name to an existing transaction.A solution might be to start a span in the
on_request
and then create later the transaction withsentry::continue_from_span
. I didn't test it though.Anyway let me know your thoughts and if there is anything you would like I change.