Skip to content

Preserve v1 span events in test-agent decoders - #12881

Open
AlexeyKuznetsov-DD wants to merge 1 commit into
masterfrom
alexeyk/fix-v1-test-decoder-span-events
Open

AlexeyKuznetsov-DD wants to merge 1 commit into
masterfrom
alexeyk/fix-v1-test-decoder-span-events

Conversation

@AlexeyKuznetsov-DD

Copy link
Copy Markdown
Contributor

What Does This Do

Fixes the test-agent trace decoders so spans sent with the v1.0 trace protocol keep their span events, and runs OpenTelemetrySmokeTest against every trace protocol.

  • JSON decoder (MessageJson/SpanJson, used with the dd-apm-test-agent): reads the native span_events field as raw JSON and exposes it as the events meta tag. A new RawJsonAdapter copies the JSON value token by token into compact text, keeping number literals verbatim (no Double round-trip for nanosecond timestamps, decimals, or integers beyond long range).
  • msgpack v1 decoder (SpanV1, used by in-process mock agents): decodes span events (field 12) instead of skipping them, into the same events tag the tracer writes for v0.4. Event names and attributes go through the string-table-aware helpers.
  • OpenTelemetrySmokeTest: now a @ClassTemplate that runs once per ProtocolVersion (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 an events meta tag but a native span field, and neither decoder read it:

  • The JSON decoder silently dropped span_events, so smoke assertions on the events tag failed under v1.0.
  • SpanV1 skipped 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.4 is the default protocol, so nothing exercised the v1.0 path end to end. Running the smoke test per protocol covers it.

Additional Notes

  • Test-only change: the decoder module and smoke tests are not part of the agent.
  • I checked locally with debug logs that each smoke run negotiates its own endpoint: v0.4/traces, v0.5/traces, v1.0/traces.
  • @ParameterizedClass cannot launch a separate app per protocol. With PER_CLASS it shares one instance across invocations, and class-level callbacks fire once per class template. So the smoke test uses a small ClassTemplateInvocationContextProvider that starts and stops each app in Before/AfterClassTemplateInvocationCallback.
  • The smoke job runs three apps instead of one.
  • Known difference: an empty span_events array decodes to "[]" in the JSON decoder but to no tag in SpanV1. TraceMapperV1 writes field 12 on every span, so SpanV1 cannot treat an empty array as meaningful.

Contributor Checklist

🤖 Generated with Claude Code

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>
@AlexeyKuznetsov-DD AlexeyKuznetsov-DD added type: bug fix Bug fix comp: testing Testing tag: no release notes Changes to exclude from release notes tag: ai generated Largely based on code generated by an AI or LLM labels Oct 14, 2026
@AlexeyKuznetsov-DD AlexeyKuznetsov-DD self-assigned this Oct 14, 2026
@AlexeyKuznetsov-DD

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-15T00:11:01.804809Z e1c9a64 Draft marked ready
🔒 Security Review ✅ Completed 2026-10-15T00:11:31.867433Z e1c9a64 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: e1c9a645c6

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: e1c9a645c6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

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

@datadog-prod-us1-5

datadog-prod-us1-5 Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

🎯 Code Coverage (details)
• Patch Coverage: 0.00%
• Overall Coverage: 59.14% (-0.56%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: e1c9a64 | Docs | Give us feedback!

@dd-octo-sts

dd-octo-sts Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite Status
Startup 🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results
Scenario Candidate master Δ (95% CI of mean)
startup:insecure-bank:iast:Agent 13.90 s 13.93 s [-0.9%; +0.4%] (no difference)
startup:insecure-bank:tracing:Agent 12.88 s 12.98 s [-1.6%; +0.1%] (no difference)
startup:petclinic:appsec:Agent 17.62 s 17.43 s [+0.3%; +1.9%] (maybe worse)
startup:petclinic:iast:Agent 16.78 s 17.50 s [-8.4%; +0.2%] (no difference)
startup:petclinic:profiling:Agent 17.25 s 16.74 s [-1.4%; +7.6%] (no difference)
startup:petclinic:sca:Agent 17.62 s 17.55 s [-0.6%; +1.5%] (no difference)
startup:petclinic:tracing:Agent 16.18 s 16.71 s [-7.5%; +1.2%] (no difference)

Commit: e1c9a645 · CI Pipeline · Benchmarking Platform UI


Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

@AlexeyKuznetsov-DD
AlexeyKuznetsov-DD marked this pull request as ready for review October 15, 2026 00:07
@AlexeyKuznetsov-DD
AlexeyKuznetsov-DD requested review from a team as code owners October 15, 2026 00:07
@AlexeyKuznetsov-DD
AlexeyKuznetsov-DD requested review from PerfectSlayer, mcculls and sarahchen6 and removed request for a team October 15, 2026 00:07

@datadog-prod-us1-5 datadog-prod-us1-5 Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Bits Code Review: PASS

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.

Was this helpful? React 👍 or 👎

Open Bits AI session

🤖 Bits Code Review · Commit e1c9a64

@PerfectSlayer PerfectSlayer left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

@mcculls

mcculls commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

I agree it would make sense to have a new smoke test for this if you want to keep the OTel ones more focused

@AlexeyKuznetsov-DD

Copy link
Copy Markdown
Contributor Author

The test agent decoder looks good but I would not change the OTel smoke tests. .... Would that be useful or scope creep?
@PerfectSlayer Thanks for the review. I agree that protocol coverage should be separate from the existing OTel smoke tests, and I’ll revert the class-template changes there.
If I understand correctly, you’re suggesting a dedicated span-events smoke test: create an OTel span with events, then verify that their names, timestamps, and attributes reach the test agent intact. Would running that focused scenario against v0.4, v0.5, and v1.0 provide the coverage you have in mind?
Since you’ve already started work in this area, would you prefer that I add the scenario here or keep this PR focused on the decoder fixes and handle the smoke test separately?

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

comp: testing Testing tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: bug fix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants