Pass parentContext to LogProcessor.emit(), similar to SpanProcessor.onStart()

[trask]

E.g. store the context passed in to LogDataBuilder.setContext() (and remove setSpanContext()), and pass that context along to LogProcessor.emit().

This would enable writing a LogProcessor that adds Baggage attributes to the log telemetry (and probably other use cases).

[jack-berg]

@trask I think you can do this today as long as the log processing is happening on the same thread and in the same context as when the log was recorded. I believe we already rely on that assumption for the log4j, logback, and jul appenders. For example here we just call setContext(Context.current()).

As far as i can tell, this was really blocked by LogRecordProcessor not being able to mutate any data, which has since been fixed. Here's a demo in action:

  @Test
  void demonstrateBaggageLogRecordProcessor() {
    InMemoryLogRecordExporter exporter = InMemoryLogRecordExporter.create();
    SdkLoggerProvider loggerProvider =
        SdkLoggerProvider.builder()
            .addLogRecordProcessor(new BaggageLogRecordProcessor())
            .addLogRecordProcessor(SimpleLogRecordProcessor.create(exporter))
            .build();

    Logger logger = loggerProvider.get("foo");

    try (Scope scope = Baggage.empty().toBuilder().put("key", "value").build().makeCurrent()) {
      logger.logRecordBuilder().setBody("hello world!").emit();
    }

    assertThat(exporter.getFinishedLogItems())
        .satisfiesExactly(
            logRecordData -> {
              assertThat(logRecordData)
                  .hasAttributes(Attributes.builder().put("key", "value").build())
                  .hasBody("hello world!");
            });
  }

  private static class BaggageLogRecordProcessor implements LogRecordProcessor {

    @Override
    public void onEmit(ReadWriteLogRecord logRecord) {
      Baggage.current()
          .asMap()
          .forEach(
              (s, baggageEntry) ->
                  logRecord.setAttribute(AttributeKey.stringKey(s), baggageEntry.getValue()));
    }
  }

[trask]

As far as i can tell, this was really blocked by LogRecordProcessor not being able to mutate any data, which has since been fixed.

👍

I think it may still be good to pass in the Context since some people prefer explicit context propagation as opposed to using ThreadLocal (though this is not a problem for auto-instrumentation, which relies on ThreadLocal).

[jack-berg]

An alternative to changing the signature of LogRecordProcessor#onEmit(..) to include Context would be to add a getter on ReadWriteLogRecord#getContext(). I think I slightly prefer adding a getter. Not sure if there is any objective reason to choose one approach over the other - they seem to offer the same function an roughly the same ergonomics.

[jkwatson]

should the blocked:spec label be removed from this issue?

[jack-berg]

The Log SDK spec LogProcesssor#onEmit method doesn't currently include Context as an argument. However, the omission is most likely just an oversight and not intentional. Its really a no-brainer to include context as an argument:

• Provides symmetry with SpanProcessor#onStart
• Allows context like baggage to be used by processor implementations without needing to rely on implicit context injection. Currently, LogRecordProcessors which wish to operate on context must rely on fetching Context.current(), which relies on a couple of assumptions about how the log was recorded and how the processor was invoked.

There's an effort to try to identify / resolve the remaining issues needed to stabilize logs, and this specific topic was included: "Consider the need for passing context as a parameter to LogRecordProcessor (from @jack-berg)". I'll be making a PR to the spec with the change, and anticipate folks will agree, so its just a matter of timing of whether to push forward with this now or wait.