This repository has been archived by the owner on Jul 26, 2024. It is now read-only.
feat: add an optional trace header to metrics/logging #146
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
This adds a `trace_header` setting. This will add the value (if found in the Headers) to the tags as `header.trace`. (e.g. if a request has `X-Cloud-Trace-Context: abc123` this will add a tag with `header.trace: abc123` to the outboud tags for logging and metrics.) Closes #145
pjenvey
reviewed
Jun 9, 2021
src/settings.rs
Outdated
| // TODO: someone better at lifetimes could fix these. | ||
| // The returned settings need only last as long as the request. | ||
| // we clone to break out of the Arc | ||
| impl From<&HttpRequest> for Settings { |
There was a problem hiding this comment.
This was trickier than I thought it was gonna be 😅 but I think this works:
feat/145-trace...feat/145-trace-lifetimes
Wanna apply the same to impl From<&ServiceRequest> for Settings below?
There was a problem hiding this comment.
Oh, cool! Thanks!
Yep, I'll modify the other ones along the same lines.
|
@jrconlin sorry, I don't understand this code so I'm not sure where to add my comment... but it's critical that the trace tag isn't added to statsd, and only added to sentry/log output. The trace id is a random string which will blow up influxdb cardinality |
|
@jbuck yeah, that's exactly the sort of comment I need. thanks! |
jrconlin
changed the title
pjenvey
previously approved these changes
Jun 10, 2021
| @@ -127,6 +129,15 @@ impl From<&RequestHead> for Tags { | |||
| extra.insert("ua".to_owned(), uas.to_string()); | |||
| } | |||
| } | |||
| if let Some(tracer) = settings.trace_header.clone() { | |||
There was a problem hiding this comment.
nit
Suggested change
| if let Some(tracer) = settings.trace_header.clone() { | |
| if let Some(ref tracer) = settings.trace_header { |
pjenvey
approved these changes
Jun 10, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Description
This adds a
trace_headersetting. This will add the value (if found inthe Headers) to the tags as
header.trace.(e.g. if a request has
X-Cloud-Trace-Context: abc123this will add atag with
header.trace: abc123to the outboud tags for logging andmetrics.)
Testing
Ensure the content of the tracing header is sent to metric and logging.
Issue(s)
Closes #145