Repository navigation
Conversation
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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
🚀 New features to boost your workflow:
|
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.
lalitb
left a comment
There was a problem hiding this comment.
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.
@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 |
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.
4e0808a to
ec30ecb
Compare
|
@lalitb Thank you for the guidance. I've implemented the solution exactly as you suggested:
The context is passed by reference and never retained. All changes are pushed and ready for review. |
…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.
|
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. |
- 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
|
@lalitb, can you confirm if all the changes requested are addressed in the later commits, and if so could you please approve the PR? |
|
Thanks for the direction. Done:
Pushed. Let me know if you'd like anything adjusted. |
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the discussion and updates. Approved with some minor requested changes below.
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
|
@dbarker Thanks for approving. I've resolved the changes:
|
|
@lalitb also could check this once? |
|
@lalitb a small reminder on this one! |
|
@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
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.
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.
|
@lalitb just a small reminder on this one! |
…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
…nt-bridge-config # Conflicts: # CHANGELOG.md
|
@lalitb could you please check this PR once? |
…nt-bridge-config # Conflicts: # CHANGELOG.md
|
@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. |
…nt-bridge-config # Conflicts: # CHANGELOG.md
…nt-bridge-config # Conflicts: # CHANGELOG.md
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.