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.
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
Init tracing #28
Init tracing #28
Changes from 2 commits
ca6d357
c0e3622
7f6ee5a
bbe02d0
b3e0dfc
f5b2c56
7498c5d
ab04384
6e11997
d4e3a1a
efde0fe
84d7ed8
6e7b555
1038334
3fbf25d
02b6025
0a798c9
3ee9e77
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Didn't see this before. Won't this add yet another layer of a span? How does it look when printed? Is it too much?
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.
oh! I think this was just me debugging/exploring. It doesn't do anything because.. yeah.. I'm not sure tbh.
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.
Actually I think if we're extracting a
req_id
for every request anyway, shouldn't we just extract it as part ofRequest::new
and store it in the request? Then theExtract
impl forReqId
can also be simplified to just take the storedReqId
from the request directly.Or instead of storing it, at least just have a method on
Request
that gets theReqId
from the delivery inside, then theExtract
impl could use that. But if we extract it every time anyway, maybe storing it isn't so bad? Any handler with an RPC client would be able to avoid extracting it twice then.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.
Yeah, I def agree with that. Seems wild to extract it every time. Also a lot easier on the eyes to just write
req.req_id()
.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.
Kind of similar but I feel this should be a method in
Request
, so this can just bereq.app_id()
.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.
It's definitely prettier ;)