Add include_trace_context property to logger builder #5366
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5366 +/- ##
============================================
- Coverage 91.20% 91.20% -0.01%
- Complexity 4866 4869 +3
============================================
Files 546 546
Lines 14373 14388 +15
Branches 1351 1352 +1
============================================
+ Hits 13109 13122 +13
- Misses 875 876 +1
- Partials 389 390 +1
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| * | ||
| * @return this | ||
| */ | ||
| LoggerBuilder excludeTraceContext(); |
There was a problem hiding this comment.
the name of this seems weird to me... since it's only about the span context, not the trace as a whole. 🤔 Is this something that should be adjusted in the spec?
There was a problem hiding this comment.
This one is a bit weird. SpanContext shows up in the trace API specification, and in our API, yet the spec log data model refers to these fields as "Trace Context Fields". I can argue both side of this:
In favor of trace context:
- Its the context that links logs to the "tracing signal"
- It contains more than just a span's context (i.e trace id and trace flags)
- W3C refers to this set of things as
In favor of span context:
- We have a SpanContext interface
- The spec refers to this bag of data (spanid, traceid, trace flags, trace state) as SpanContext
Given that the log data model is stable, can we change the log data model to refer to "Span Context Fields" instead of trace context fields? If so, we could / should change this to include_span_context instead of include_trace_context.
@tigrannajaryan curious to get your thoughts on this.
There was a problem hiding this comment.
I think we have been using Trace Context and Span Context interchangeably a lot. Span Context is probably a slightly more accurate term since it matters which span in a trace we are referring to from a log. I don't mind renaming this in the spec and in the data model and I don't think it is a breaking change, to me it is just a terminology clarification, we should be able to do it and there are no functional changes.
There was a problem hiding this comment.
Actually, one thing that may prevent us from doing it is that we consider it to be valid for a Log Record to reference a trace_id but have no associated span_id. You won't encounter it in Otel-generated data but some weird external data sources like this may be possible so we allowed for it in the data model.
There was a problem hiding this comment.
I think we have been using Trace Context and Span Context interchangeably a lot.
That's been my experience as well. Here are usage of trace context, tracecontext, span context and spancontext in the specification.
I don't feel strongly either way about this one.
There was a problem hiding this comment.
If span context and trace context are used interchangeably, its probably not a big deal for otel java to name this excludeSpanContext instead, given that we have a SpanContext interface which would be good to reference in the javadoc.
There was a problem hiding this comment.
how about just "excludeContext" in case we might want to also start pulling in baggage or something later?
| * Specify that emitted log records SHOULD NOT automatically include trace context from the {@link | ||
| * Context#current()}. |
There was a problem hiding this comment.
I wonder if we should say "span context" rather than "trace context" (since I don't think that's a thing outside of the W3C propagator). Also, should we include a link to the SpanContext class in the javadoc for completeness?
|
FYI, we're considering removing |
|
No longer need as the spec dropped this parameter in #3397. |
The spec for LoggerProvider's "Get a Logger" operation includes a
include_trace_contextparameter, which defaults to true. Ifinclude_trace_context=true, then the SpanContext field of emitted LogRecords should be extracted from the current context. If false, then the SpanContext field of emitted LogRecords should be extracted from the explicitly set context, or otherwise invalid.I've haven't included this so far because the use case is rather esoteric and we haven't seen users ask for it. Its used in situations where the emitted log records are coming from threads which may have active spans, but which are not related to the emitted log records. Its probably more useful in event scenarios. But you can also imagine valid logging scenarios, where a log framework has its own holder for span context and deriving SpanContext from current span context is invalid.
Anyway, that's the backstory.
I've opened this PR because the spec says the "API MUST accept the following parameter". However, we could add it in a backwards compatible way after an initial stable release. Additionally, there are other cases where our implementation doesn't include all the "MUST" elements of the specification. Our ReadWriteLogRecord has placeholders for reading / writing fields.