Change Log flags field to TraceFlags #3811
Conversation
jack-berg
requested review from
anuraaga,
bogdandrutu,
carlosalberto,
jkwatson and
jsuereth
as code owners
| @@ -47,7 +48,7 @@ | |||
| LogData.builder(RESOURCE, InstrumentationLibraryInfo.create("instrumentation2", "2")) | |||
| .setName("testLog2") | |||
| .setBody("body2") | |||
| .setFlags(0) | |||
| .setTraceFlags(TraceFlags.getDefault()) | |||
There was a problem hiding this comment.
might be nice to have one example that's sampled, as well. That goes for all the tests in this PR.
There was a problem hiding this comment.
There are a lot of tests that create LogData but most of the functionality under test doesn't concern the trace flags field directly. I'd prefer to limit testing of the different shapes of trace flags to the tests that are directly concerned with serializing / deserializing trace flag values. That would include OtlpJsonLoggingLogExporter, LogsRequestMarshalerTest, which tests serialization of LogData to proto, and OtlpExporterIntegrationTest, which tests exporting to the collector and back to a java process.
If we have tests that validate the correctness of the low level code, then we can trust that it will work for higher level code that consumes it.
There was a problem hiding this comment.
sounds good to me. TraceFlags aren't a mandatory field, correct? We could just leave them out of the high level tests altogether in most cases, I'd assume.
There was a problem hiding this comment.
Instead of three methods, should it be setSpanContext?
There was a problem hiding this comment.
Another conceptual question, I don't think we have trace flags on metrics exemplar. It feels like the spancontext information added to a log would match what's added to an exemplar.
There was a problem hiding this comment.
Yeah I don't think its worth merging the PR if its just going to change in the near future.
Some additional food for though:
- The Log4j2 OpenTelemetryContextDataProvider currently associates span context with Log4j2 log events.
- In my Log4j2 appender prototype, I read those fields out of the context map and associate them with the
LogBuilder(note I haven't extracted theTRACE_FLAGSfield yet). - If we depend on the log appenders calling
Context.current()to get context, rather than first injecting the context into the log as is done in Log4j2, then we are assuming that the appenders are called synchronously from the same thread when alogger.log(...)is invoked, right? Because if the appender is not called synchronously on the same thread, thenContext.current()won't result the right context. I think.
There was a problem hiding this comment.
Interesting - the context provider can't inject Context itself because it needs to set a string map. So if we rely on the injection than baggage would also all need to be injected as key value pairs. Any plans for that out of curiosity?
I have a feeling that expecting the OTel appender to be synchronous is going to be more practical but not sure.
There was a problem hiding this comment.
So if we rely on the injection than baggage would also all need to be injected as key value pairs. Any plans for that out of curiosity?
I haven't heard anything about logs and baggage. @tigrannajaryan do you have any insight into this?
I have a feeling that expecting the OTel appender to be synchronous is going to be more practical but not sure.
Log4j2 has the AsyncAppender, which allows you to wrap other appenders and causes log events to be written to them on a separate thread. We'd have to tell users they can't wrap the otel appender in an AsyncAppender. It would be a bit strange, but not outright wrong, for a user to use the otel appender with the AsyncAppender, since BatchLogProcessor already exports on a separate thread.
Logback has the same concept.
There was a problem hiding this comment.
Yes, I've been worrying about async appenders since the very early days of the logging project. I don't know that we have any good solution for that.
There was a problem hiding this comment.
We discussed in the 11/4 Java + Instrumentation Office hours and decided to have a LogBuilder#setContext(Context) method and LogData#getSpanContext().
Codecov Report
@@ Coverage Diff @@
## main #3811 +/- ##
============================================
+ Coverage 89.22% 89.39% +0.17%
- Complexity 4010 4017 +7
============================================
Files 481 481
Lines 12430 12432 +2
Branches 1207 1207
============================================
+ Hits 11091 11114 +23
+ Misses 926 910 -16
+ Partials 413 408 -5
Continue to review full report at Codecov.
|
|
Replacing this PR with #3837, which includes the changes we discussed in the 11/4 Java + Instrumentation Office Hours. |
Part of Issue #3804.
Adjust the log
int getFlags()field toTraceFlags getTraceFlags().