Repository navigation
Preserve v1 span events in test-agent decoders - #12881
AlexeyKuznetsov-DD wants to merge 1 commit into
Conversation
Spans decoded from v1 payloads lost their span events in both test-agent decoders. The JSON decoder now reads the agent's native span_events field as raw JSON, keeping number literals verbatim, and exposes it as the events meta tag. The msgpack SpanV1 decoder skipped the span events field, which also consumed streaming strings without registering them and desynchronized the string table. It now decodes the events through the table-aware helpers into the same events meta tag the tracer writes for v0.4. OpenTelemetrySmokeTest now runs once per trace agent protocol (v0.4, v0.5 and v1.0), so the v1 span events and links path is covered end to end even though v0.4 is the default protocol. (cherry picked from commits aabb490 and c385b63) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
There was a problem hiding this comment.
More details
The event decoder matches the writer’s schema and preserves streaming string-table state. The smoke-test callbacks give each protocol its own app lifecycle; runtime execution remains unverified.
🤖 Bits Code Review · Commit e1c9a64
There was a problem hiding this comment.
The test agent decoder looks good but I would not change the OTel smoke tests.
I re-implemented them to be as simple as possible for easy maintenance so I would rather not have ClassTemplate, invocation context provider, etc... mixed with it as 1. OTel instrumentation testing is not related to trace protocol testing (otherwise, better test OTLP protocol), 2. the OTel smoke tests doesn't have span events.
I had started adding the smoke test testing capability for span events on the side too (that's part of my OKR) and I planned to add OTel span events dedicated testing scenario.
Would you like to start new kind of smoke test for that part in particular? Would that be useful or scope creep?
|
I agree it would make sense to have a new smoke test for this if you want to keep the OTel ones more focused |
|
What Does This Do
Fixes the test-agent trace decoders so spans sent with the
v1.0trace protocol keep their span events, and runsOpenTelemetrySmokeTestagainst every trace protocol.MessageJson/SpanJson, used with the dd-apm-test-agent): reads the nativespan_eventsfield as raw JSON and exposes it as theeventsmeta tag. A newRawJsonAdaptercopies the JSON value token by token into compact text, keeping number literals verbatim (noDoubleround-trip for nanosecond timestamps, decimals, or integers beyondlongrange).SpanV1, used by in-process mock agents): decodes span events (field 12) instead of skipping them, into the sameeventstag the tracer writes forv0.4. Event names and attributes go through the string-table-aware helpers.OpenTelemetrySmokeTest: now a@ClassTemplatethat runs once perProtocolVersion(0.4,0.5,1.0), each run with its own app and test-agent backend.Motivation
With
v1.0, span events are no longer aneventsmeta tag but a native span field, and neither decoder read it:span_events, so smoke assertions on theeventstag failed underv1.0.SpanV1skipped the field blindly. Besides losing the events, this consumed streaming strings without registering them, putting the string table out of sync. Later string indexes in the payload then resolved to the wrong strings or failed.v0.4is the default protocol, so nothing exercised thev1.0path end to end. Running the smoke test per protocol covers it.Additional Notes
v0.4/traces,v0.5/traces,v1.0/traces.@ParameterizedClasscannot launch a separate app per protocol. WithPER_CLASSit shares one instance across invocations, and class-level callbacks fire once per class template. So the smoke test uses a smallClassTemplateInvocationContextProviderthat starts and stops each app inBefore/AfterClassTemplateInvocationCallback.span_eventsarray decodes to"[]"in the JSON decoder but to no tag inSpanV1.TraceMapperV1writes field 12 on every span, soSpanV1cannot treat an empty array as meaningful.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue🤖 Generated with Claude Code