[CONFIGURATION] Implement EventToSpanEventBridge log record processor

[om7057]

Fixes #4155

Implements ExperimentalEventToSpanEventBridgeLogRecordProcessor from the declarative configuration spec. Mirrors the EventToSpanEventBridge processor already in the Java SDK: if a log record has an event name and its trace/span id match the currently active span, it gets copied over as a span event on that span. Doesn't export anything itself and doesn't stop the record from continuing through the rest of the pipeline.

YAML key is event_to_span_event_bridge/development, matching the schema in open-telemetry/opentelemetry-configuration (checked opentelemetry_configuration.json and meta_schema_language_cpp.yaml, which list this type as not_implemented with no extra properties).

New processor class under sdk/include/opentelemetry/sdk/logs/event_to_span_event_bridge_processor.h + .cc, wired into the config parser, the visitor, and SdkBuilder the same way simple/batch already are.

Tests: sdk/test/logs/event_to_span_event_bridge_processor_test.cc covers bridging onto the current matching span, ignoring records with no event name, and ignoring events from a different span. sdk/test/configuration/yaml_logs_test.cc covers the YAML parsing side.

Ran the full local test suite (915 tests) before pushing, all green. CHANGELOG updated.

[codecov]

Codecov Report

❌ Patch coverage is 64.70588% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.23%. Comparing base (f1e1c9c) to head (7696c6b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...k/src/logs/event_to_span_event_bridge_processor.cc	53.69%	44 Missing ⚠️
sdk/src/configuration/sdk_builder.cc	0.00%	5 Missing ⚠️
...gs/event_to_span_event_bridge_processor_factory.cc	0.00%	3 Missing ⚠️
...ation/log_record_processor_configuration_visitor.h	0.00%	2 Missing ⚠️
...ry/sdk/logs/event_to_span_event_bridge_processor.h	50.00%	2 Missing ⚠️
sdk/src/logs/logger.cc	90.48%	2 Missing ⚠️
api/include/opentelemetry/logs/logger.h	90.00%	1 Missing ⚠️
sdk/src/logs/multi_log_record_processor.cc	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4309      +/-   ##
==========================================
- Coverage   82.35%   82.23%   -0.12%     
==========================================
  Files         502      506       +4     
  Lines       19877    20044     +167     
==========================================
+ Hits        16368    16481     +113     
- Misses       3509     3563      +54     

Files with missing lines	Coverage Δ
..._event_bridge_log_record_processor_configuration.h	100.00% <100.00%> (ø)
...lude/opentelemetry/sdk/configuration/sdk_builder.h	100.00% <ø> (ø)
sdk/include/opentelemetry/sdk/logs/processor.h	100.00% <100.00%> (ø)
sdk/src/configuration/configuration_parser.cc	80.15% <100.00%> (+0.47%)	⬆️
sdk/src/logs/logger_context.cc	94.60% <100.00%> (+0.66%)	⬆️
api/include/opentelemetry/logs/logger.h	81.45% <90.00%> (+0.34%)	⬆️
sdk/src/logs/multi_log_record_processor.cc	96.00% <93.75%> (-0.42%)	⬇️
...ation/log_record_processor_configuration_visitor.h	50.00% <0.00%> (-50.00%)	⬇️
...ry/sdk/logs/event_to_span_event_bridge_processor.h	50.00% <50.00%> (ø)
sdk/src/logs/logger.cc	93.45% <90.48%> (-0.67%)	⬇️
... and 3 more

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[om7057]

@dbarker @marcalff could you please review this PR?

[lalitb]

As mentioned here - . This violates the Logs SDK specification for explicitly supplied contexts and can silently drop valid events. The current C++ LogRecordProcessor::OnEmit() API does not expose that context, so the API gap
needs to be addressed before this bridge can be implemented correctly.

[om7057]

How do we know the ambient span at OnEmit() is the span from the log's resolved context?

The Logs SDK specification requires using the resolved context: the explicitly supplied context when present, otherwise the ambient context. These can be different. For example, the log may be emitted with span_a explicitly while span_b is currently ambient. In that case this processor finds span_b, its IDs do not match the log, and the event is silently dropped.

The current LogRecordProcessor::OnEmit() API does not receive the resolved context, so I don't think Tracer::GetCurrentSpan() can implement the required behavior correctly.

@lalitb Thank you for pointing this out. The current implementation doesn't handle explicitly supplied contexts properly; it will silently drop events from logs that were emitted with a different span context than what's currently active.

Would it be acceptable to:

Add a method to the Recordable interface to store/retrieve the resolved context
Update Logger to set it when creating the recordable
Have the bridge use it to properly match against the log's context
This way we keep existing processor signatures unchanged while giving processors access to the context they need. What do you think?

[om7057]

@lalitb Thank you for the guidance. I've implemented the solution exactly as you suggested:

• Added OnEmitWithContext(record, context) to LogRecordProcessor with a default implementation that forwards to OnEmit() for backward compatibility
• Updated Logger to call OnEmitWithContext() with the resolved context
• Updated MultiLogRecordProcessor to forward the context to child processors
• Implemented EventToSpanEventBridge to override OnEmitWithContext() and use the resolved context for proper span matching

The context is passed by reference and never retained. All changes are pushed and ready for review.

[om7057]

Thanks for the thorough review @ThomsonTan.

Fully addressed: the compatibility documentation and the visitor source break, the type-aware YAML assertion (plus a misspelled-key guard), and the duplicated emit pipeline.

Partially addressed, with one honest caveat: the discarded-span bug is fixed in the processor and covered by a regression test that fails against the old code, and ordinary dispatch is back on OnEmit() with no added GetCurrent() for context-ignoring pipelines. But the explicit context still never reaches the processor end-to-end, because Logger::EmitLogRecord() doesn't receive it, so in practice the bridge is still handed the ambient context. I've left details and a question on the relevant threads.

Still open: the eager child recordable. I couldn't find a fix that actually avoids the copy and would like your steer before touching MultiRecordable; details on that thread.

Verified locally; please check once more that the changes have been made.

[om7057]

@lalitb, can you confirm if all the changes requested are addressed in the later commits, and if so could you please approve the PR?

[om7057]

@lalitb, just a small reminder: the merge conflicts have been resolved after addressing the request changes please have a look at them.

[om7057]

@ThomsonTan @lalitb Everything else from the review is addressed. One thing is still open: this question on whether to add the ABI-v2-guarded context_or_span plumbing from logger.h down to the processor in this PR, or land the processor-level fix now and track that as a separate follow-up.

Would like to know what can be preferred!

[om7057]

@lalitb small ping on this

[lalitb]

@om7057 - The explicit context is still lost here. If the log uses span_a while span_b is ambient, this passes span_b to the processor and the event is silently dropped.

Please carry the already resolved context through the logger emit path and add an end-to-end test with explicit span_a and ambient span_b.

[om7057]

Thanks for catching; the resolved context was getting lost. EmitLogRecord(args...) computed context_or_span but then called the record-only overload, which re-derived it from RuntimeContext::GetCurrent() instead of using the one already resolved. Exactly the span_a/span_b case you described.

Fixed in 7696c6b: added Logger::EmitLogRecordWithContext() (distinctly named, default forwards to EmitLogRecord() so existing implementations are unaffected) and threaded the resolved context through to it. Added ContextAwareProcessorReceivesExplicitContextNotAmbientSpan, which reproduces your scenario and fails against the old code. Verified in a fresh ABI v2 build (my local builds were all ABI v1) plus the full ABI v1 suite

[om7057]

@lalitb I've addressed the comment, please check once again!

[dbarker]

Thanks for digging into this! The bridge processor is a nice driver to get these interfaces aligned with the spec.

Since this is a major API driving change for both the logger API and the logs SDK. I think it is worth breaking those changes out to a new PR with more analysis and design documentation accounting for currently supported otel-cpp use cases and spec requirements. It is possible that some of the ABIv2 logger interfaces should change to address passing context explicitly through to the processors.

Once those interfaces are settled and merged the bridge processor can use them.

Please see feedback below with some of the use cases that we need to support or explicitly break. Sharing links to the relevant specs:

https://opentelemetry.io/docs/specs/otel/logs/api/#emit-a-logrecord
https://opentelemetry.io/docs/specs/otel/logs/sdk/#onemit
https://opentelemetry.io/docs/specs/otel/context/

[dbarker]

Once the context is passed through EmitLogRecord to processor OnEmit, it should be done so unconditionally. This extra virtual method here is not necessary and should be removed.

[om7057]

Understood on removing the gate for spec compliance, want to flag a conflict before I do since it affects the design of the follow up PR.

ThomsonTan's earlier review on this same code asked for the opposite: resolving the context costs a RuntimeContext::GetCurrent() call, and that shouldn't happen on the hot path for pipelines (simple/batch) that never read it, hence the ConsumesResolvedContext() gate.

If the spec requires the resolved context to always reach OnEmit(), the cost has to be paid unconditionally, which reopens that concern. A couple of options for the new PR to consider:

• Pay the cost always, as you're describing. Spec says so, this is likely correct.
• Or resolve lazily: pass something like a thunk or callable that resolves RuntimeContext::GetCurrent() only if a processor actually invokes it, so processors that ignore the parameter never pay for the lookup.

Which direction should the new design PR pursue?

[dbarker]

Calling emit with the resolved context is a spec requirement and should occur unconditionally. ConsumesResolvedContext should be removed as an unnecessary API expansion of the processor interface.

As @lalitb points out the Context must be resolved during the API EmitLogRecord calls (it is either explicitly passed in by the user or the current context is implicitly used).

We need to account for all supported logger API use cases and document what the expected behavior will be with the new bridge processor for each.

Here are the uses cases that need to be supported from the API.

Context original_context;
Context context = trace::SetSpan(original_context, my_span); 

// Cases 1-4 conform to the spec since a full Context is resolved and 
// a span ptr can be accessed from the processor. 

// Case 1: call emit with context resolved to explicit context
logger->EmitLogRecord("with explicit context", context);

// Case 2: call emit with context resolved to current context
logger->EmitLogRecord("with implicit current context"); 

// Case 3: call create then emit with ABIv1. Context is resolved to current context OR to 
auto record_abiv1 = logger->CreateLogRecord();
logger->EmitLogRecord(std::move(record_abiv1); 

// Case 4: call create then emit with ABIv2. Context is resolved to the explicit context
auto record_abiv2 = logger->CreateLogRecord(context); 
logger->EmitLogRecord(std::move(record_abiv2); 
 

// Cases 5-7 are supported by the otel-cpp API but don't comply with the spec since a full context cannot be effectively resolved in a way that would make the underlying span ptr accessible.  

SpanContext span_context;        // a user-managed SpanContext (ids + flags only, no live Span)
TraceId trace_id;
SpanId  span_id;
TraceFlags flags;

// Case 5: explicit SpanContext
logger->EmitLogRecord("with span context", span_context);

// Case 6: explicit TraceId + SpanId (+ optional TraceFlags) 
logger->EmitLogRecord("with trace/span id parts", trace_id, span_id, flags);

// Case 7: ABI v2 create-then-emit with explicit SpanContext
auto record_w_span_context= logger->CreateLogRecord(span_context);
logger->EmitLogRecord(std::move(record_w_span_context));

[om7057]

Thank you for these cases, this is exactly the kind of matrix that was missing. Replied on the other thread about the ConsumesResolvedContext tension with ThomsonTan's feedback, so I will not repeat that here.

On cases 1 to 4, agreed those are the clean spec compliant ones and should just work once the context passing is unconditional.

On cases 5 to 7, I think the honest answer is that a bare SpanContext or TraceId plus SpanId cannot resolve to a live Span, so no processor can get a Span pointer out of them no matter how we wire things. My current bridge implementation falls back to comparing against the ambient span's ids in that case, meaning the event only gets added if the ambient span happens to be the one the ids refer to. That is a real, documented limitation rather than a bug, since the API genuinely gives us nothing to look up a Span by id.

Case 7 looks like it has a second version of the same bug though. CreateLogRecord(span_context) stamps the record's trace and span ids, but nothing carries span_context forward to the later EmitLogRecord(record) call, so that call falls back to resolving the ambient context again, same as case 3. If that is correct then case 7 needs the same fix as the case this PR started from, just for the create then emit pattern instead of the single call pattern.

Also want to flag that case 3's comment cuts off after "Context is resolved to current context OR to", might be missing the rest of that sentence.

I will pull all of this into the new PR's design doc once we're aligned on the ConsumesResolvedContext direction, want to avoid designing it piecemeal across comments here.

[dbarker]

Per comments above I'm requesting that this PR be broken up into:

1. A PR with Logger API and SDK changes and tests for spec compliant context passing through emit to the processors. More design and analysis is needed.
2. A PR (this existing one is fine) with only the bridge processor and declarative config support

[om7057]

Thanks @dbarker, that makes sense. I'll split it: bridge processor and declarative config stay here, and I'll open a new PR for the Logger API and SDK context passing work with the use case matrix you laid out documented against the spec.

Before I start on that PR, I'd like to get alignment on one open tension (left a comment on the ConsumesResolvedContext() thread). ThomsonTan's earlier feedback on this same PR was that resolving context should be opt-in (only when a processor needs it) to avoid a RuntimeContext::GetCurrent() call on every log for pipelines that don't care. Your comment says the opposite: it should be unconditional per spec, and ConsumesResolvedContext() should go away. I want to design the new PR around whichever of these is actually correct rather than build it twice.