This repository has been archived by the owner on Dec 6, 2024. It is now read-only.
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.
add conventions for log correlation #114
add conventions for log correlation #114
Changes from 3 commits
f8cc2cf
0a724be
ea90540
058ae05
3a33eb3
a86472b
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.
Are "Trace correlation" and "Request correlation" interchangeable and equivalent terms?
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 think the answer is no, but I need some help here- I think that "trace correlation" is a David-ism, so I was about to rename it to "request correlation", but not all traces are part of a request- do we have a more general term that we use? Or is request correlation correct in this context?
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 don't have an answer, no preference here. Not sure if have a standardized terminology for this already. Perhaps check the rest of the spec and see if there is one.
If there is none let's choose one and stick with 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.
"traceflags" is not explained anywhere above. I think it would be useful to tell what it is about.
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.
At Uber we only record the
sampled
Boolean in the logs. Is the fulltraceflags
field needed?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.
At the moment sampled is the only flag defined in the W3C spec, so it's effectively the same.
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.
This refers to base-16 encoded string. However, what if my logging medium is binary and allows proper representation of arbitrary by sequences? Do I still have to use base-16 strings or I can just emit the bytes? If this recommendation is for text medium (such as text log files) it may be worth calling out specifically.
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 think that the underlying presumption with this document is that if certain fields are set in certain ways, we will be able to recognize them and use them when the log is processed. I don't think that's going to be true one way or another with a binary representation, so I'll add the text-based caveat to the introduction.
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.
The semantics of Required are unclear here. Some logs may be emitted when there is no trace context present, so they cannot contain trace-id. And the whole feature of trace/log correlation is technically optional.
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'll clarify, thanks.
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.
At least in Python, the log format requires all fields to be present on the log record object or it throws an exception. Also, the log format cannot be changes dynamically depending on whether an active span (or fields on log record) is present or not. So in this case we can either leave the fields empty so that the formatted logs look like
trace_id= span_id=
or we can set them to zero so it looks liketrace_id=0 span_id=0
. Can we clarify what values the fields should be set to in case no active span is found in the context?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.
@owais what would you prefer? Also, it seems like a fairly specific use case, could it be left to whoever configures the logger on how they want to represent the lack of trace context?
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 have a slight preference for
0
.It could but then backends will have to deal with multiple ways of representing nil values (0 vs '' vs null in JSON, etc). It would be nicer if the spec recommended one thing when the field cannot be omitted completely so analysis tools can have a standard way of detecting 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.
It is not quite clear what this means. What level of variation is acceptable? Typically, where it matters we use RFC kewords MAY, SHOULD, MUST, etc to specify how strict the particular part of the spec is and what variation is allowed.