DO NOT MERGE - test the converting-agent PR against all system-tests - #7339
DO NOT MERGE - test the converting-agent PR against all system-tests#7339ajgajg1134 wants to merge 16 commits into
Conversation
|
|
|
…de origin The trace agent's v1 conversion feature made three debugger test groups fail, all because the tests still assumed the legacy agent payload shape. Exception replay (java): a converted span carries a `_dd.convertedv1` provenance marker, and v0.4 `meta` values stay strings where a natively emitted v1 payload keeps real booleans. Drop the marker and canonicalize booleans on both sides of the approval comparison, so a single set of approval files stays valid whether or not conversion is enabled. Normalizing is preferred over re-recording the baselines, which would bake the current conversion path into them. Code origin (python): `_wait_for_code_origin_span` indexed the raw payload with legacy-only keys (`tracerPayloads`, `resource`, `type`, `meta`), so under v1 it never matched a span and timed out into a misleading "code origin not enabled" assertion. Iterate `interfaces.agent.get_spans()` instead, whose typed wrappers resolve those fields for both formats. Exception replay (php): the `*_spans_expected_v1.json` approvals were never generated, so the tests raised FileNotFoundError. Recorded them with DI_OVERRIDE_APPROVALS so they land in the existing 1.18.0 folder alongside the legacy files, rather than in a new version folder. Validated locally per language with the converting agent and with the default agent, confirming the payload format in each run via the captured agent interface files: java/spring-boot DEBUGGER_EXCEPTION_REPLAY 8 passed both ways python/flask-poc DEBUGGER_INPRODUCT_ENABLEMENT 5 passed both ways php/apache-mod-8.0 DEBUGGER_EXCEPTION_REPLAY 9 passed both ways Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-run CI after widespread SSI matrix and PHP e2e job failures.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e3f04c641
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if isinstance(data, bool): | ||
| return "true" if data else "false" |
There was a problem hiding this comment.
Limit boolean normalization to span attributes
When a v1 payload incorrectly encodes a schema field such as the top-level error flag as the string "true", this recursive conversion changes the approval's Boolean and the received string to the same value, so the approval test passes despite the malformed span. The format difference described here applies to legacy meta values carried into v1 attributes; restrict canonicalization to those attributes so envelope and span-field types remain validated.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
All seven new PHP v1 approval files keep _dd.p.tid, but the scrub code removes this tag before comparison. Each affected exception replay approval comparison therefore fails.
🤖 Datadog Autotest · Commit 2e3f04c · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
…vertingAgent # Conflicts: # tests/debugger/test_debugger_exception_replay.py
_dd.p.tid was added to the scrub-removal set in test_debugger_exception_replay.py after these approval files were generated, so they still carried the key as "<scrubbed>" instead of having it removed, causing comparison failures.
The efficient trace payload format (idxTracerPayloads) merges meta and metrics into a single typed `attributes` map, so a tag a tracer reports both as a string tag and as a numeric metric collides, and the numeric value is the one that survives. The Rust tracer does exactly this for http.status_code, which made Test_Config_HttpServerErrorStatuses_Default fail against the converting agent with `assert 400.0 == '400'`, while every other library passed. Add DataDogAgentSpan.get_tag(), which normalizes integral numbers back to their string form for that format only. The legacy format keeps meta and metrics separate, so values are returned untouched there and a numeric value in meta remains a failure, as it should. Verified in replay mode against the rust/axum CI artifact: the two failing tests pass, the full DEFAULT scenario matches CI exactly (67 passed / 386 skipped / 43 xfailed / 3 xpassed) with no other change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| @@ -0,0 +1 @@ | |||
| datadog/agent-dev:andrew-glaude-convert-by-default-on-55244-py3 No newline at end of file | |||
There was a problem hiding this comment.
You can achieve the same by adding [agent@andrew-glaude-convert-by-default-on-55244-py3] in your PR title.
### What does this PR do?
Enables the "v1.0" trace payload conversion by default. `handleTraces` now
routes through the V1 (idx) pipeline unless the new `disable-convert-traces`
feature flag is set, inverting the previous opt-in `convert-traces` flag.
**Note that much of this has been previously reviewed but was reverted due to unexpected breakages in system-tests which have since been resolved**
OTLP receiver is left out to minimize changes and reduce review burden.
Three latent bugs that the default flip exposes are fixed here:
- `Get128BitTraceID` parsed all 32 hex characters of `otel.trace_id` as a
`uint64`, which always overflowed. Only the first 16 hold the upper bits.
Reachable on any span the datadog exporter sends to the normal HTTP
endpoints, since `ConvertToIdx` now runs on that traffic.
- `TraceWriterV1` dropped the `_dd.otel_gateway` payload tag that the legacy
writer sets.
- `idx.InternalSpan` had no `String()`, so a log line interpolating one
rendered as `%!s(*idx.Span=&{...})`.
The rest is test migration to the idx format: the trace-agent testsuite, new
`idxTracePayload` helpers in the fakeintake client, and the apm, ssi,
installer and agent-platform e2e suites.
### Motivation
The conversion path has been opt-in behind `convert-traces` since #53339. This
makes it the default so it gets real traffic, while keeping
`disable-convert-traces` as the escape hatch. We will remove the disable flag once we have sufficient success after release and following from that will remove the old code paths entirely :)
### Describe how you validated your changes
- `dda inv test --targets=./pkg/trace/api,./pkg/trace/agent,./pkg/trace/writer,./pkg/proto/pbgo/trace,./pkg/serverless/trace,./comp/trace/payload-modifier/impl,./cmd/serverless-init/trace` — 1300 pass, 5 skipped
- `bazel build //cmd/trace-agent/test/testsuite:all //comp/otelcol/otlp/integrationtest:all //test/fakeintake/client:all //pkg/trace/api:all //pkg/trace/writer:all` — success
- `bazel run //:gazelle` — no remaining BUILD.bazel churn
- New unit tests for each fix: `span_utils_test.go` (32-hex `otel.trace_id`),
`tracev1_test.go` (`otel_gateway` tag), `internal_span_test.go` (`String()`)
- We've been running this enabled internally for quite some time without incident
- I manually triggered the APM System-tests DataDog/system-tests#7339 and validated that all the system-tests pass.
### Additional Notes
Co-authored-by: andrew.glaude <andrew.glaude@datadoghq.com>
cbeauchesne
left a comment
There was a problem hiding this comment.
Another not-well-known feature : the run-all-scenarios label. So you don't need to add a dummy file to trigger them.
Motivation
Changes
Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teambuild-XXX-imagelabel is present