Conversation
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 5 Pipeline jobs failed
|
| Test | New execution time | Base Execution time | Increase | DataDog link |
|---|---|---|---|---|
testScenario with data set "A simple GET request returning a string"from tests/Integrations/Symfony/V5_0.DDTrace\Tests\Integrations\Symfony\V5_0\CommonScenariosTest.DDTrace\Tests\Integrations\Symfony\V5_0\CommonScenariosTest::testScenario |
6.04s | 559.585161ms | +5.48s (+980%) | View in Datadog |
testScenario with data set "A simple GET request returning a string"from tests/Integrations/Symfony/V3_0.DDTrace\Tests\Integrations\Symfony\V3_0\CommonScenariosTest.DDTrace\Tests\Integrations\Symfony\V3_0\CommonScenariosTest::testScenario |
4.97s | 766.453667ms | +4.2s (+548%) | View in Datadog |
tmp/build_extension/tests/ext/pcntl/pcntl_fork_thread_mode_orphan.phpt (Thread mode sidecar: orphaned child process promotes itself to master after parent exits)from PHP.tmp.build_extension.tests.ext.pcntl |
3.97s | 731.107011ms | +3.24s (+443%) | View in Datadog |
tmp/build_extension/tests/ext/background-sender/sidecar_thread_mode_permissions.phpt (Thread mode sidecar uses abstract Unix socket)from PHP.tmp.build_extension.tests.ext.background.sender |
16.02s | 1.48s | +14.54s (+985%) | View in Datadog |
testScenario with data set "A simple GET request returning a string"from tests/Integrations/Symfony/V5_1.DDTrace\Tests\Integrations\Symfony\V5_1\CommonScenariosTest.DDTrace\Tests\Integrations\Symfony\V5_1\CommonScenariosTest::testScenario |
3.95s | 565.601005ms | +3.38s (+598%) | View in Datadog |
ℹ️ Info
🎯 Code Coverage (details)
• Patch Coverage: 86.67%
• Overall Coverage: 68.41% (+0.02%)
Useful? React with 👍 / 👎
This comment will be updated automatically if new data arrives.🔗 Commit SHA: 61c85ee | Docs | View more details | Give us feedback!
Benchmarks [ tracer ]Benchmark execution time: 2026-09-30 16:10:43 Comparing candidate commit c24031b in PR branch Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥 Scenarios present only in baseline:
Found 12 performance improvements and 23 performance regressions! Performance is the same for 154 metrics, 1 unstable metrics.
|
2531886 to
b29cd49
Compare
10c7edf to
84351df
Compare
73c262e to
0d3d03d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d3d03d6f7
ℹ️ 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".
Leiyks
changed the title
f553cb8 to
a8f30bd
Compare
c653af1 to
7b533f7
Compare
Benchmarks [ appsec ]Benchmark execution time: 2026-10-01 18:03:34 Comparing candidate commit 61c85ee in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics.
|
| // Global tags (DD_TAGS, add_global_tag) are attribute defaults: one still at its default yields to a | ||
| // deprecated $meta value for the same key, as when the defaults lived in meta. | ||
| static void dd_yield_global_defaults_to_meta(zend_array *attributes, zend_array *meta, zend_array *globals) { |
There was a problem hiding this comment.
Can we please discuss the exact fallback behaviour?
I'm not sure how this merges.
We have a couple choices:
- Make meta/metrics/attributes references to each other. -> They have the same contents, are overlapping, type expectations probably violeted.
- Do whatever you did (which is a bit off to me?) -> I don't fully understand it.
- Just copy all values in meta and metrics into attributes at serialization time. -> if you call
unset($span->meta["..."]);it'll no longer unset nor will it find any integration provided value if accessed in a custom hook. - Return an object quacking like an array/iterable, which actually delegates to attributes, whenever meta or metrics is accessed. -> a bit more complex, but probably most faithful?
I would tend to try the last option, and it should allow us to completely leave this concern out of the serializer. It becoming now a self-contained concern on the span.
There was a problem hiding this comment.
I mean sure the last solution seems nice but the question to me is more: what the precedence order when attributes, meta and metrics happens to have the same key.
What I am doing now is putting priority on attributes since the other 2 will be deprecated.
There was a problem hiding this comment.
The last solution sidesteps this? It provides a view onto attributes, so attributes is always the source of truth. There's no precedence to respect here then.
There was a problem hiding this comment.
But putting priority on attributes is exactly the wrong way round - if attributes take precedence and an user assigns to meta in his existing code ... well, his assignments will just be ignored after updating.
| smart_str_free(&combined); | ||
| } else { | ||
| ddog_add_str_span_meta_zstr(rust_span, "_dd.tags.process", process_tags); | ||
| dd_span_attr_zstr(rspan, "_dd.tags.process", process_tags); |
There was a problem hiding this comment.
Please ask around whether process tags should be chunk level spans in v1 or remain as first span.
11cd443 to
f0e1de3
Compare
|
The span stats entrypoints are service names. See https://github.com/DataDog/datadog-agent/blob/12897bea576e821511abcfd9dd4f2cf06cc04f1e/pkg/trace/traceutil/trace.go#L135-L158 for how the agent decides (which we mirror). It's also related to chunking_. So technically we should not take into account already submitted spans. Please rollback these last two commits fully. |
…er FFI Serialize closed spans into a libdatadog TracerPayloadV1Builder (Box-per-node pointer handles) and send them to the sidecar as /v1.0/traces, with the sidecar negotiating /info and downgrading to v0.4 for agents that lack it. The in-process sender keeps the v0.4 wire, transcoded from the same builder. Span tags live in a single typed attributes store; SpanData::$meta and $metrics are views onto it. Process tags and the OTLP export marker are TracerPayload attributes (the v0.4 downgrade puts them on each chunk's first span), and span links and events carry native attributes. DD_TRACE_AGENT_PROTOCOL_VERSION=0.4 forces v0.4. Bump libdatadog to the V1 FFI branch and port components-rs to the new rate limiter API.
Use typed SpanData::$attributes and the promoted env/version/component/span_kind properties instead of $meta/$metrics string conversions in the userland integrations and the OpenTelemetry bridge. Drop the userland App Analytics plumbing (addTraceAnalyticsIfEnabled, requiresExplicitTraceAnalyticsEnabling, markForTraceAnalytics and the per-integration candidates). TraceAnalyticsProcessor::normalizeAnalyticsValue and Tag::ANALYTICS_KEY are now deprecated no-ops; the serializer still maps an analytics.event tag to _dd1.sr.eausr.
0a2b3fb to
24f8c4c
Compare
Snapshots difference summaryThe following differences have been observed in committed snapshots. It is meant to help the reviewer. If you need to update snapshots, please refer to CONTRIBUTING.md |
Leiyks
changed the title
dd_trace_serialize_closed_spans() now reports the V1 shape: a typed attributes map with promoted fields (span_kind, component, sampling_priority, ...) and native span links and events. Migrate the phpt, PHPUnit and snapshot tests to it, and add coverage for the V1 and v0.4 wires, span attributes, ignoreError and the default root component. Remove tests for the retired userland msgpack serializer and thread sender (dd_trace_serialize_msgpack, dd_trace_send_traces_via_thread, the in-process background sender capability tests) and for the removed App Analytics trace search config.
Add a msgpack V1 decoder to the request-replayer and advertise /v1.0/traces in its /info, so the PHP test suite can inspect V1 payloads. Move the CI service to the 5.0 image and force-pull the Windows service images so a runner with a stale cached digest does not skip a rebuilt image.
…merge Clone system-tests at SYSTEM_TESTS_REF (default leiyks/php-v1-payload, DataDog/system-tests#7843) from SYSTEM_TESTS_REPO, and add APM_TRACING_EFFICIENT_PAYLOAD to the System Tests matrix.
24f8c4c to
61c85ee
Compare
2 participants
Migrate the tracer to the V1 Efficient Trace Payload protocol (APMLP-1197). Depends on DataDog/libdatadog#2311: the
libdatadogsubmodule is pinned to that PR's head and must be re-pinned to the merge commit once it lands./v1.0/traces. The sidecar negotiates/infoand downgrades to v0.4 when the agent does not advertise/v1.0/traces. The newDD_TRACE_AGENT_PROTOCOL_VERSION=0.4forces v0.4._dd.top_leveland sendsclient_computed_top_level; the snapshots now include it.SpanData::$metaand$metricsare views onto the new$attributes, with promoted$env/$version/$component/$spanKindproperties, native nested attributes for span links and events, andmeta_structas bytes. Process tags (_dd.tags.process) and the OTLP export marker (_dd.sdk.otlp_export) are payload attributes;_dd.git.*is set on the payload and on root spans.dd_trace_serialize_closed_spans()returns the V1 shape on every PHP version and the tests are migrated to it.dd_trace_serialize_msgpack()anddd_trace_send_traces_via_thread()are removed, along with their tests and the background-sender tests built on them./v1.0/traces.Behaviour changes users will notice:
DD_TRACE_SIDECAR_TRACE_SENDER=0opts back into the in-process sender.componentto the SAPI name (frameworks still override it).TraceAnalyticsProcessor::normalizeAnalyticsValueis a deprecated no-op,Tag::ANALYTICS_KEYis deprecated, ananalytics.eventtag is still mapped to_dd1.sr.eausr, and the TraceSearch config tests are removed.DD_TRACE_WARN_LEGACY_DD_TRACEis removed.For reviewers:
ci(temp): run system-tests from leiyks/php-v1-payload, points the system-tests job at [php] Enable v1 payload tests, and make trace assertions format-agnostic system-tests#7843 and must be reverted before merge.dockerfiles/services/request-replayersource; rebuild it if that source changes.test_tags_defaults_sst002is markedbug (APMAPI-1545)there for the sampling mechanism 0 issue.