Skip to content

[CONFIGURATION] Implement EventToSpanEventBridge log record processor - #4309

Open
om7057 wants to merge 53 commits into
open-telemetry:mainfrom
om7057:fix/event-to-span-event-bridge-config
Open

om7057 wants to merge 53 commits into
open-telemetry:mainfrom
om7057:fix/event-to-span-event-bridge-config

Conversation

@om7057

@om7057 om7057 commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

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.

Adds the ExperimentalEventToSpanEventBridgeLogRecordProcessor type from
the declarative configuration spec. When a log record carries an event
name and matches the trace and span id of the currently active span,
this processor copies it over as a span event, mirroring the existing
implementation in the Java SDK.

Fixes open-telemetry#4155
@om7057
om7057 requested a review from a team as a code owner July 26, 2026 05:12
@codecov

codecov Bot commented Jul 26, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.56%. Comparing base (3ee38aa) to head (fd4b5e5).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4309      +/-   ##
==========================================
+ Coverage   86.51%   86.56%   +0.05%     
==========================================
  Files         530      531       +1     
  Lines       21138    21150      +12     
==========================================
+ Hits        18286    18306      +20     
+ Misses       2852     2844       -8     
Files with missing lines Coverage Δ
..._event_bridge_log_record_processor_configuration.h 100.00% <100.00%> (ø)
...ation/log_record_processor_configuration_visitor.h 100.00% <ø> (ø)
sdk/src/configuration/configuration_parser.cc 84.33% <100.00%> (+0.44%) ⬆️
sdk/src/configuration/logs_builder_utils.cc 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

om7057 added 3 commits July 26, 2026 11:27
The new yaml_logs_test.cc test case used dynamic_cast, but one CI config
builds with -fno-rtti; switched to reinterpret_cast to match the rest of
the file. Also reverted an unrelated indentation change in sdk_builder.cc
that a local clang-format 14 run introduced on lines I didn't touch;
CI runs clang-format 18, which formats these continuation lines
differently.
clang-tidy flagged three things that pushed the abiv2-preview preset
over its warning budget: the attributes_ member stored a reference
instead of a pointer, OnEmit released the incoming unique_ptr without
an explicit std::move, and ForEachKeyValue could let an allocation
failure escape a noexcept override. Switched the member to a pointer,
added std::move, and wrapped the iteration in a try/catch guarded by
OPENTELEMETRY_HAVE_EXCEPTIONS, matching the pattern already used in
periodic_exporting_metric_reader.cc.

iwyu wanted <utility> in both files for std::move, plus a few trace
headers directly included in the test file rather than pulled in
transitively.
…ectly

configuration_parser.cc used EventToSpanEventBridgeLogRecordProcessorConfiguration
via a transitive include only, which iwyu flagged in both abiv1-preview and
abiv2-preview jobs.
@om7057

om7057 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

@dbarker @marcalff could you please review this PR?

@lalitb lalitb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

om7057 commented Jul 28, 2026 •

Copy link
Copy Markdown
Contributor Author

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 added 3 commits July 28, 2026 19:56
Add OnEmitWithContext() method to LogRecordProcessor interface to pass the
resolved context (explicit if provided, else ambient) to processors that need it.
The context is valid only for the duration of the call and must not be retained.

Updates:
- LogRecordProcessor: new OnEmitWithContext() virtual method (default forwards to OnEmit)
- Logger: call OnEmitWithContext with resolved context
- MultiLogRecordProcessor: implement OnEmitWithContext to forward to child processors

This enables processors like EventToSpanEventBridge to access the log's resolved
context for proper span matching, addressing spec compliance for explicit contexts.
…pport

Update EventToSpanEventBridgeProcessor to override OnEmitWithContext and use the
log's resolved context (explicit if provided, else ambient) for proper span matching.

Changes:
- Override OnEmitWithContext to receive the resolved context from the logger
- Extract span context from the variant (SpanContext or Context)
- Compare resolved span context to log's trace/span IDs
- Verify current active span matches resolved context before adding event
- Update processor docstring to clarify context usage

This ensures spec compliance for explicitly-supplied log contexts where the log
and ambient span might differ.
@om7057
om7057 force-pushed the fix/event-to-span-event-bridge-config branch from 4e0808a to ec30ecb Compare July 29, 2026 18:48
@om7057

om7057 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

@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.

Comment thread sdk/src/logs/logger.cc Resolved
om7057 added 3 commits July 30, 2026 07:35
…nt-bridge-config

# Conflicts:
#	CHANGELOG.md
…n, harden tests

Bridge onto the span from the resolved context (ThomsonTan review):
OnEmitWithContext() looked up the live span from the resolved Context and then
discarded it in favour of the ambient span, so an event whose ids referred to an
explicitly supplied span was dropped whenever a different span was ambient. The
span from the resolved Context is now the span the event is added to. A bare
SpanContext carries no live Span, so that alternative is handled separately and
still resolves through the ambient span, with the id check rejecting a mismatch.

Remove the duplicated emit pipeline: OnEmit() and OnEmitWithContext() had drifted
apart. Both now share BridgeRecordToSpan(), which holds all preconditions and the
emission in one place.

Keep the emit hot path free of context resolution: LogRecordProcessor gains
ConsumesResolvedContext() (default false), MultiLogRecordProcessor reports true
when any child does, and LoggerContext caches the answer the same way it already
caches RecordableEnforcesLogRecordLimits(). Logger only resolves the ambient
context and dispatches through OnEmitWithContext() when some processor will
actually read it, so simple/batch pipelines keep dispatching through OnEmit()
with no extra RuntimeContext::GetCurrent() call.

Document compatibility: OnEmitWithContext() and ConsumesResolvedContext() are
source compatible via default implementations but change the LogRecordProcessor
vtable, so subclasses must be recompiled. Noted in the header and in the
CHANGELOG breaking-changes section. VisitEventToSpanEventBridge() was pure
virtual, which broke existing visitors, and now defaults to a no-op.

Make the YAML assertion type-aware: the reinterpret_cast only repeated the
preceding non-null check and passed for a misspelled key that fell through to the
extension configuration. The test now dispatches through Accept() with a
recording visitor, and a companion test pins the misspelled-key fallback.

Tests: explicit span_a with ambient span_b asserting only span_a receives the
event (fails against the previous code), explicit-context mismatch, bare
SpanContext, and Logger-level tests that a context-ignoring processor dispatches
through OnEmit() and adds no GetCurrent() call, using a counting
RuntimeContextStorage.
The base declaration left the second parameter unnamed while the comment
documented @PARAM context, so Doxygen warned that the documented argument was
not in the argument list -- once for LogRecordProcessor and again for the
EventToSpanEventBridgeProcessor override that inherits the comment.

Name the parameter and mark it OPENTELEMETRY_MAYBE_UNUSED, since the default
implementation forwards to OnEmit() without reading it. Keeps the parameter
documented rather than dropping the @PARAM line.

Verified with doxygen against docs/public/Doxyfile.lint: no warnings remain for
any file this PR touches. The warnings still reported for api logs logger.h,
noop.h and sdk resource.h are pre-existing on main, are not emitted by the
Doxygen version CI uses, and are untouched here.
@om7057

om7057 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

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 added 3 commits July 31, 2026 02:19
- event_to_span_event_bridge_processor.cc: drop <memory>, no longer needed
  directly now that the shared BridgeRecordToSpan() helper pulls unique_ptr
  transitively through recordable.h.
- event_to_span_event_bridge_processor_test.cc: swap trace/context.h for
  nostd/string_view.h + trace/span_metadata.h; the new regression tests use
  trace_api::kSpanKey directly and never call GetSpan()/SetSpan().
- logger_sdk_test.cc: add stddef.h (size_t) and nostd/unique_ptr.h
  (CountingRuntimeContextStorage::Attach's return type), both used directly
  by the new dispatch tests.
- yaml_logs_test.cc: swap the two concrete
  {event_to_span_event_bridge,extension}_log_record_processor_configuration.h
  includes for the base log_record_processor_configuration.h; the parsed
  processor pointer is typed as the base class and only Accept() is called on
  it, the concrete types are only named as visitor-callback parameters, which
  the visitor header already forward-declares.

Verified with 'iwyu' locally is not available in this environment, so
confirmed by inspection against the CI diagnostics and by rebuilding: full
build is clean and ctest is 931/931 passing. doxygen docs/public/Doxyfile.lint
still reports zero warnings for any file this PR touches.
clang-tidy (both abiv1-preview and abiv2-preview pushed the warning count
over the job's threshold by 2):

- event_to_span_event_bridge_processor.cc:237 bugprone-exception-escape:
  OnEmitWithContext() is noexcept but used holds_alternative<Context>() +
  get<Context>(context), and nostd::get() throws bad_variant_access on a
  mismatched alternative. Even though the holds_alternative check made that
  path unreachable, clang-tidy can't prove it statically. Switched to
  nostd::get_if<Context>(&context), which returns nullptr instead of
  throwing -- the same pattern Logger::ExtractSpanContext already uses for
  this exact variant in logger.cc.

- logger_sdk_test.cc:1014 cppcoreguidelines-rvalue-reference-param-not-moved:
  DispatchRecordingProcessor::OnEmit() took its unique_ptr<Recordable>&&
  by dropping it unnamed. Named the parameter and moved it into a
  static_cast<void>(...), mirroring the sibling OnEmitWithContext() override
  a few lines below that already does this.

iwyu (abiv2-preview only; abiv1/abiv1-preview already required these
directly and passed):

- logger_sdk_test.cc: marked the <stddef.h> and opentelemetry/context/
…nt-bridge-config

# Conflicts:
#	CHANGELOG.md
@om7057

om7057 commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor Author

@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

om7057 commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the direction. Done:

  • Opened [SDK] Support non-exporting pipeline log record processors (needed for EventToSpanEventBridgeProcessor) #4454 to track the SDK implementation, referencing the architecture discussion and use case matrix from this PR, marked blocked pending the read-write recordable / composable pipeline processor / root processor work.
  • Added a FIXME-SDK comment in SdkBuilder::CreateEventToSpanEventBridgeLogRecordProcessor linking to that issue.
  • Scoped this PR down to just the configuration model and YAML parser: the parser still creates the EventToSpanEventBridgeLogRecordProcessorConfiguration model element, and SdkBuilder now logs a warning and skips building a processor rather than throwing or crashing, so a pipeline like [event_to_span_event_bridge, batch] still runs batch normally.
  • Removed the actual EventToSpanEventBridgeProcessor implementation and its tests, and reverted the Logger/LogRecordProcessor resolved-context plumbing (EmitLogRecordWithContext, OnEmitWithContext) since it existed only to serve that processor. It'll come back once the new pipeline architecture is in place and someone implements the processor against it.

Pushed. Let me know if you'd like anything adjusted.

@dbarker dbarker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the discussion and updates. Approved with some minor requested changes below.

Comment thread sdk/src/configuration/sdk_builder.cc Resolved
Comment thread CHANGELOG.md Resolved
Comment thread CHANGELOG.md Resolved
om7057 added 2 commits August 20, 2026 08:18
Per @dbarker:
- VisitEventToSpanEventBridge() is now pure virtual instead of defaulting to
  a no-op. Configuration classes under this directory are internal
  (used only by tests and SdkBuilder) and experimental, so this breaking
  change is fine without a changelog entry.
- Shortened the FIXME-SDK comment in sdk_builder.cc to just point at the
  tracking issue, leaving implementation details to the issue itself.
- Trimmed the CHANGELOG entry to describe only what this PR changes.
…nt-bridge-config

# Conflicts:
#	CHANGELOG.md
@om7057

om7057 commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor Author

@dbarker Thanks for approving.

I've resolved the changes:

  • VisitEventToSpanEventBridge is now pure virtual (both implementers already overrode it, so this was a clean change).
  • Shortened the FIXME-SDK comment in sdk_builder.cc to just the tracking issue link.

@om7057

om7057 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

@lalitb also could check this once?

@dbarker
dbarker requested a review from lalitb August 21, 2026 15:18
@om7057

om7057 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

@lalitb a small reminder on this one!

@dbarker dbarker removed the discuss To discuss in SIG meeting label Sep 4, 2026
@dbarker

dbarker commented Sep 4, 2026

Copy link
Copy Markdown
Member

@lalitb This PR has been descoped to just the YAML parsing and model. The SdkBuilder will log a warning if a user configures the events to spans bridge processor to indicate that the feature is not supported. This is consistent with how the SDK reports unsupported yaml elements from schema 1.1.0. Please take a look since your change request is currently blocking PR.

…nt-bridge-config

# Conflicts:
#	CHANGELOG.md
…nt-bridge-config

# Conflicts:
#	sdk/test/configuration/sdk_builder_test.cc
@dbarker dbarker added the pr:fix-merge-conflicts Please fix merge conflicts for this pr label Sep 11, 2026
main split SdkBuilder's per-signal creation methods out into
LogsBuilderUtils/MetricsBuilderUtils/TraceBuilderUtils (static
methods over a Registry*) while this branch was open. Ported the
event-to-span-event-bridge processor's warn-and-skip creation
function and its LogRecordProcessorConfigurationVisitor dispatch
into logs_builder_utils.cc at the new location.
@marcalff marcalff removed the pr:fix-merge-conflicts Please fix merge conflicts for this pr label Sep 16, 2026
clang-format expects includes sorted alphabetically; the newly added
event_to_span_event_bridge_log_record_processor_configuration.h
include was placed after extension_log_record_processor_builder.h
instead of before extension_log_record_exporter_builder.h.
@om7057

om7057 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

@lalitb just a small reminder on this one!

om7057 added a commit to om7057/opentelemetry-cpp that referenced this pull request Sep 24, 2026
…nally

Split out of open-telemetry#4309 per review from dbarker and ThomsonTan: the
EventToSpanEventBridgeProcessor added there needs the resolved trace
Context at OnEmit time to reach the live Span it bridges an event onto.
Getting that right turned out to be public API surface for both
logs::Logger and sdk::logs::LogRecordProcessor, so it gets its own PR
with its own analysis before the bridge processor depends on it.

The bug: Logger::EmitLogRecord(record) re-derived the resolved context
from RuntimeContext::GetCurrent() (ambient) instead of reusing whatever
context had already been resolved for this record, so an explicitly
supplied context could be silently dropped in favor of whatever span
happened to be ambient at emit time.

Design, evaluated against the 7 use cases below: resolve context once
and always pass it to OnEmit, no per-processor opt-in.

* api/include/opentelemetry/logs/logger.h (ABI v2): added
  EmitLogRecordWithContext(record, resolved_context), a new virtual
  with a default that forwards to EmitLogRecord(record) so existing
  Logger implementations keep compiling unchanged. Distinct name
  (not an EmitLogRecord() overload) so a derived class overriding
  only the record-only overload can't hide it. EmitLogRecord(args...)
  now calls it with the context_or_span it already resolved for the
  Enabled() filter chain, instead of discarding that resolution.
  Extracted the per-argument stamping loop (previously duplicated)
  into a shared StampLogRecordFields() helper.

* sdk/include/opentelemetry/sdk/logs/processor.h: added
  OnEmitWithContext(record, context), unconditional (no opt-in flag),
  default forwards to OnEmit() so unrelated processors are unaffected.

* sdk/src/logs/logger.cc: EmitToProcessor() always calls
  OnEmitWithContext(). When no context was ever supplied to this
  specific call (the plain EmitLogRecord(record) path), it resolves
  ambient fresh as a best-effort fallback.

* sdk/{include,src}/logs/multi_log_record_processor.{h,cc}: fans
  OnEmitWithContext() out to every child processor.

The 7 use cases (matrix from review), and how each resolves:

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

  1. EmitLogRecord("msg", context)          -- explicit Context.
  2. EmitLogRecord("msg")                   -- implicit ambient.
  3. CreateLogRecord(); EmitLogRecord(r)     -- no context anywhere.
  4. CreateLogRecord(context); EmitLogRecord(r) -- explicit to Create only.
  5. EmitLogRecord("msg", span_context)     -- bare SpanContext.
  6. EmitLogRecord("msg", trace_id, span_id, flags)
  7. CreateLogRecord(span_context); EmitLogRecord(r)

Cases 1/2/5/6 already resolve context inside EmitLogRecord(args...)
regardless of any processor -- passing it through costs nothing new.

Cases 3/4/7 use the two-step create-then-emit pattern, which the
Logger has no way to carry an explicit context across: the resolved
Context (and any live Span it owns) is not retained anywhere between
the two calls. Storing it on the Recordable to bridge that gap was
considered and rejected -- BatchLogRecordProcessor buffers a
Recordable for up to scheduled_delay_millis (default 5000ms) before
export, so a Context stashed there would keep its Span alive for
that whole window, an unrelated subsystem silently extending a
span's lifetime. Reconstructing a SpanContext from the record's own
stamped fields was also considered and rejected -- the base
Recordable interface is write-only (no GetTraceId/GetSpanId), so
recovering it generically would need either an unsafe downcast or
expanding that interface for every processor's own Recordable
implementation.

So case 3 (no explicit context ever given) resolves ambient fresh at
emit time -- spec-correct, since ambient-at-emit-time is the right
answer when nothing explicit was supplied. Cases 4 and 7 (explicit
context/span_context given to Create only) are a documented,
tested limitation: the plain EmitLogRecord(record) call sees
ambient, not the original context. Callers who need the processor to
see the same context they gave CreateLogRecord must call
EmitLogRecordWithContext(record, context) instead, re-passing the
context they already have in hand. Case 7 is not actually a
regression versus case 5/6: a bare SpanContext never carried a live
Span to begin with, so the ids delivered via
EmitLogRecordWithContext are full compliance for that case.

Verified etw::Logger (exporters/etw/include/opentelemetry/exporters/etw/etw_logger.h),
the one real production logs::Logger subclass besides the SDK's own:
it only overrides the record-only EmitLogRecord(), not
EmitLogRecordWithContext, so it is unaffected and keeps its existing
ambient-fallback behavior. No production LogRecordProcessor subclass
exists outside sdk/src/logs/ (checked exporters/otlp, elasticsearch,
ostream, opentracing-shim).

Tests: sdk/test/logs/logger_sdk_test.cc adds ContextCapturingProcessor
and one test per case above, including the two documented-limitation
tests for cases 4/7 and their EmitLogRecordWithContext-fixed
counterparts, plus three tests pinning the exact GetCurrent() call
count for cases 1/2/3 (0, 1, and 2 respectively) to make the cost
claim in this description verifiable rather than asserted.

Verified in a from-scratch ABI v2 build (WITH_ABI_VERSION_1=OFF
WITH_ABI_VERSION_2=ON) as well as the existing ABI v1 build: full
ctest suites green in both (904/904 and 1367/1367), clang-format
applied, doxygen and misspell clean.
…nt-bridge-config

# Conflicts:
#	CHANGELOG.md
@om7057

om7057 commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor Author

@lalitb just following up, this has been ready for your review since @dbarker's approval on Aug 19.
Let me know if there's anything blocking or if you'd like changes.

@om7057

om7057 commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

@lalitb could you please check this PR once?

…nt-bridge-config

# Conflicts:
#	CHANGELOG.md
@om7057

om7057 commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor Author

@marcalff could you please check this PR once? It has an approval from dbarker, and lalitb hasn't had a chance to get back to it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CONFIGURATION] File configuration - EventToSpanEventBridge

5 participants