add conventions for log correlation #114
Conversation
zenmoto
requested review from
arminru,
bogdandrutu,
c24t,
carlosalberto,
iredelmeier,
jmacd,
reyang,
SergeyKanzhelev,
tedsuo,
tigrannajaryan and
yurishkuro
as code owners
There was a problem hiding this comment.
@zenmoto thanks for starting this document, this is very important for supporting modern observability in applications that emit standalone logs in existing formats.
text/logs/0114-log-correlation.md
Outdated
|
|
||
| ### Request Correlation | ||
|
|
||
| Trace correlation is achieved primarily with two values- a trace identifier and |
There was a problem hiding this comment.
Are "Trace correlation" and "Request correlation" interchangeable and equivalent terms?
There was a problem hiding this comment.
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.
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.
text/logs/0114-log-correlation.md
Outdated
| ## Trade-offs and mitigations | ||
|
|
||
| As a convention, adherence to these standards may have some variation, and some | ||
| of this advice may be difficult to implement in certain circumstances. |
There was a problem hiding this comment.
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.
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
|
@open-telemetry/specs-approvers @open-telemetry/specs-logs-approvers please review this. |
|
open-telemetry/opentelemetry-specification#1283 changes the format of |
|
it also seems that correlation context should be renamed to baggage? cf open-telemetry/opentelemetry-specification#536 and open-telemetry/opentelemetry-specification#857 |
|
|
||
| | Field | Required | Format | ||
| | :--------- | :------- | :-------------------------------------------------- | ||
| | traceid | Yes | 16-byte numeric value as Base16-encoded string |
There was a problem hiding this comment.
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 like trace_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.
@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.
I have a slight preference for 0.
could it be left to whoever configures the logger on how they want to represent the lack of trace context?
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.
| redundant. In certain cases it may be important to associate this context with | ||
| log entries. When the context is embedded in a log entry, the key-value | ||
| pairs should be placed in a 'ctx' namespace. Where key-value pairs are | ||
| supported, embed the correlation key as “correlation.key_name”. In JSON or |
There was a problem hiding this comment.
| supported, embed the correlation key as “correlation.key_name”. In JSON or | |
| supported, embed the correlation key as “ctx.key_name”. In JSON or |
|
Most of the proposals in this PR are already part of the specification. This OTEP is actually someone out of sync / behind with the specification. I suggest that we close the OTEP and any additional clarifications necessary are submitted as issues/PRs against specification repo. |
I know this is an aged pull here, but just wanted to point out that at least as of today the open-telemetry/opentelemetry-specification/specification/logs/overview.md doc still refers to this pull for definitions on log correlation behavior. For posterity, I think what you're saying is that folks interested learning more/contributing to the log correlation spec should instead refer to and issue requests against the data-model.md doc/area, is that right? This area is also mentioned, but its confusing. It seems like removing the mention to this pull would be a quick win for anyone else interested in following along. |
|
@tigrannajaryan the spec still refers to this OTEP: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/README.md#trace-context-in-legacy-formats Should we go ahead and approve it? |
This OTEP's ideas were incorporated in to the spec, so it served its purpose, however the spec slightly deviated from this OTEP in nuances. I will close this OTEP and it can stay for posterity. The spec probably needs slight re-wording to ensure it does not imply that the OTEP is the source of truth. |
@zenmoto do you think you can condense this OTEP into 1-2 paragraphs that we can incorporate into the spec? Linking to the OTEP is a bit misleading (e.g. OTEP uses |
|
The important parts of this OTEP have made their way into the spec. Closing this PR as it's outdated. |
This OTEP is to add basic conventions for correlating logs with traces and resources by modifying existing logging formats.