Conversation
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 19 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_1.DDTrace\Tests\Integrations\Symfony\V5_1\CommonScenariosTest.DDTrace\Tests\Integrations\Symfony\V5_1\CommonScenariosTest::testScenario |
4.01s | 570.70423ms | +3.44s (+603%) | View in Datadog |
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 |
5.9s | 562.192453ms | +5.34s (+950%) | 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.59s | 766.529271ms | +3.82s (+499%) | 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 |
4.03s | 730.089139ms | +3.3s (+452%) | View in Datadog |
ℹ️ Info
🎯 Code Coverage (details)
• Patch Coverage: 86.67%
• Overall Coverage: 68.29% (+0.02%)
Useful? React with 👍 / 👎
This comment will be updated automatically if new data arrives.🔗 Commit SHA: 11cd443 | Docs | View more details | Give us feedback!
Benchmarks [ tracer ]Benchmark execution time: 2026-09-25 16:14:30 Comparing candidate commit 14e0236 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 1 performance improvements and 33 performance regressions! Performance is the same for 155 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
…vert OrphansTest over-migration, re-bless dm snapshots - Bump libdatadog for the span_links/span_events v0.4-downgrade legacy meta fix (_dd.span_links / events, byte-identical to the pre-native tracer's json_encode output) and mirror the parity test updates in components-rs/bytes.rs. - SpanChecker: reconstruct sampling_mechanism/sampling_priority/origin/ trace_id_high only on the local-root span (they're chunk/trace-level on the real v0.4 wire, not per-span); accept span links via either the native top-level span_links or the legacy _dd.span_links meta. - Revert OrphansTest's round-1 over-migration: sampling_priority reads the real wire response (metrics._sampling_priority_v1), not the introspection-only promoted key. - tests/ext/dd_trace_span_data_serialization_with_links.phpt: expect the native trace_id_high on links. - Re-bless the 72 snapshots whose root _dd.p.dm serializes "-0" (was "0") now that the root-only gate is correct.
…_events dual-wire tests, harness fixes - serializer.c: trace_id_high/sampling_priority/sampling_mechanism/origin emitted only on the chunk's local-root span (via the new ddog_v1_get_chunk_root_span_idx FFI), matching the wire encoder; drops the flat chunk_root fallback. - SpanChecker: reconstruct _dd.origin/_dd.p.tid/_dd.p.dm/ _sampling_priority_v1 wherever present, no ad-hoc gate. - Revert the dm snapshot re-bless (72 files); span_events phpts assert both legacy and v1 wire shapes. - CLITestCase/LongRunningScriptTest: match /v1.0/traces too. - generate-tracer.php: pull the Windows test_c docker images before the service containers. - 29 phpt EXPECT updates for the removed chunk_root field. - Bump libdatadog to 5a7e2448f (shared local_root_idx chunk-root rule, chunk-root FFI, span.kind=internal fix).
…op dd_span_sink - Drop the redundant _dd.p.tid meta delete and inferred-span transfer; the v0.4 downgrade already filters it. - Remove dd_span_sink: serialization returns the ddog_SpanNode (NULL = dropped), chunk writes use ctx->chunk, 1:1 wrappers replaced by direct ddog_* calls. - Link/event attributes are always native: drop the JSON fallback and the float parity gate; objects become public-property maps, nested nulls are skipped. - Recursion guards on link/event and span paths, keyed on the object for objects; fixes crashes on nested [] (write to the immutable empty array) and on self-referencing DateTime. - Bump libdatadog: legacy v0.4 link/event JSON prints floats exactly like json_encode.
…init test - PHP 7 `attributes` is initialized in the create_object callbacks (as meta/metrics), so the ddtrace_init_span/dd_alloc_span_stack copies are no-ops; remove them. - span_data_attributes_init.phpt: cover InferredSpanData, a userland subclass, a write, and tracer-created spans/stacks.
- replace the top-down AttrBuilder (parent pointers + ddog_attr_close) with ddog_AttrList/ddog_AttrMap: new(capacity) -> fill -> push into parent / set on span/link/event - distinct list vs map builder types - explicit cycle handling in native emit - bump libdatadog for the VecMap insertion-order fix
…rsion guard cleanup - SpanData::$attributes is the primary input (attributes > meta > metrics); $meta/$metrics @deprecated; C writers/readers, shared-key src/ + appsec sites and DDTrace\Span/OTel moved to attributes; typed span int/bool FFI. - error.ignored replaced by bool $ignoreError (the tag is still honoured). - Recursion guard: one Z_*_RECURSION_P path (<7.3 shim dispatches on type), skip immutable arrays + addref while iterating (fixes an opcache SHM crash), dd_native_container removed. - #[Trace] tags: cycle guard, and the empty-tags immutable-array crash fixed.
- Exec: write the shell tags into attributes (they were serialized after
them, reordering the loader test output).
- appsec endpoint_fallback/api_security fixtures and profiling runtime_id
tests use ->attributes for tracer-written keys (attributes > meta).
- appsec telemetry.c: drop a double blank line (clang-format).
- serializer_wire_sidecar_v1.phpt: resend until the span arrives in any
chunk of a v1 request (sidecar /info negotiation, batched payloads).
- close_spans_until/span_on_close/die_in_sandbox: %s instead of {%S},
which pecl run-tests doesn't support.
- supported-configurations: DD_TRACE_SIDECAR_TRACE_SENDER default true is
a new registry version (B).
The runner still forced the in-process sender on PHP < 8.3, so it never flushed the sidecar that its CLI children now use by default, and the last trace of a CLI script could miss the snapshot (kafka).
origin/master carries three schemars versions, so cargo rewrites the unqualified reference the merge kept.
The flag is reserved for the agent: the test agent answers such payloads
with 400 "Tracers must not set the droppedTrace(5) flag", which made
system-tests lose traces ("got None"). The chunk priority already carries
the sampling decision, so the v0.4 downgrade is unchanged.
Bumps libdatadog for the removed getter.
c653af1 to
7b533f7
Compare
…true/false - Bump the request-replayer CI image tag 4.0 -> 5.0 (dockerfiles/services/.env, .gitlab/generate-common.php) so CI builds a fresh image from our replayer source, which now includes the v1 decoder and a v1-advertising /info. - ExecIntegration::createSpan: strval(true) rendered "1"; map bools to "true"/"false" to match datadog_convert_to_str's rendering of the old meta path. Add cmd.truncated assertions to the shell/exec truncation tests. - ExecIntegration::proc_close post-hook: drop the $span->meta fallback, which is never populated on the attributes-based path. - serializer_wire_sidecar_v1.phpt: derive span_name/span_service from the actually-matched span instead of $req's truthiness, and drop the tautological has_chunks line, so both remaining EXPECT lines can fail.
Benchmarks [ appsec ]Benchmark execution time: 2026-09-28 13:12:57 Comparing candidate commit b4a841f in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics.
|
| /// See [`ddog_link_set_trace_id`]. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn ddog_link_add_attr_str( | ||
| link: *mut SpanLinkBytes, |
There was a problem hiding this comment.
Instead of having 3 sets (links, events, spans), we could just use ddog_attributes_add_str(). And add a fn ddog_link_get_attributes(link: &mut SpanLinkBytes) -> &mut VecMap (same for span and events).
Would also trivially reduce duplication when serializing.
| // 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.
…s FFI
SpanData::$attributes is the single tag store. $meta and $metrics become
views onto it (DDTrace\SpanTagsView): writes coerce to strings/doubles,
unset/isset/reads act on attributes, reads return an array snapshot, and
whole-array assignment replaces that bucket. The last write wins, like
master, so the attributes > meta precedence machinery goes away (DD_TAGS
yield rule, dual lookups, has-attr guards, meta/metrics serializer loops,
C and appsec fallbacks). AppSec is back to master's single-array code.
The per-node attribute setters become one family on an opaque
ddog_Attributes handle (ddog_{span,link,event}_get_attributes,
ddog_attr_map_get_attributes, ddog_attributes_add_*), and the span, link,
event and nested-map emitters collapse into dd_attr_emit.
Per the V1 tracer guide, process tags are sent as a TracerPayload attribute, taken from the payload's first trace. The introspection view and the request-replayer V1 decoder show it on each trace's first span, as the v0.4 downgrade does (replayer image 5.0 -> 6.0). Also covers `$span->meta ?? []` and isset after an ->attributes write in the view test.
…sing, span kind
- DD_TRACE_AGENT_PROTOCOL_VERSION ("1.0" default = V1 when /info advertises it,
"0.4" forces v0.4), passed to the sidecar session
- payload env/app_version/hostname/git from the payload's first root span, as
the root-span tags resolve them (hostname only with DD_TRACE_REPORT_HOSTNAME,
git only with DD_TRACE_GIT_METADATA_ENABLED)
- sampling_mechanism from the digits after the last '-' of _dd.p.dm; unset
when unparseable
- explicit span.kind "internal" is the Internal kind, not a duplicate tag;
unset kinds stay unspecified (introspection omits span_kind)
- libdatadog 4135f42d1 (top-level spans on the V1 send, SpanKind::Unspecified)
…nternal" Explicit "internal" is now sent once as the V1 kind (no duplicate attribute), so the decoder writes it back like the v0.4 downgrade does. Tag stays 5.0.
… migrations - dd_tags_replace_bucket updates kept keys in place, so `$span->meta += [...]` and whole-bucket assignment no longer move existing tags. - Tests changed only for the ->meta/->metrics -> ->attributes migration are back to master, now that the views make ->meta/->metrics work again. - Stale expectations: span_kind (Unspecified default) in the appsec, loader and <7.4-only phpts; attribute order and meta/metrics view blocks re-blessed.
…sidecar _dd.top_level in SpanChecker
Match master: a string analytics.event is parsed as a Go ParseBool (invalid values emit nothing), other values go through zval_get_double, a valid value wins over a set _dd1.sr.eausr, and analytics.event itself is removed. A user-set _dd1.sr.eausr is kept. The integration analytics processor stays removed.
…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.
Web entrypoint root spans had no component unless a framework set one. Default it to sapi_module.name (add-if-absent, not for cli), like other tracers' server-layer component (net/http, http, wsgi, rack).
The root web.request span now defaults its component tag to the SAPI name (345d817). Update the PHPUnit/.phpt/.groovy assertions that previously expected no component / framework:unknown on non-framework root spans.
2 participants
Migrate the tracer to the V1 Efficient Trace Payload protocol.
dd_span_sinkfinalization body (no v04 intermediate): promotedenv/version/component/span.kind, chunk-level 128-bittrace_id+sampling_mechanism, unified typedattributes(incl.meta_structas bytes), nativespan_links/span_events./infodoesn't advertise/v1.0/traces, the sidecar transcodes v1→v0.4 (existing libdatadog encoder). Isolated + removable — dropping v0.4 later = delete the/infocheck + the transcode call.dd_trace_serialize_closed_spans) returns the v1 shape uniformly on all versions;DD_TRACE_AGENT_PROTOCOL_VERSIONgate removed./v1.0/traces;tests/extexpectations updated to v1 (603 passing).Depends on libdatadog #2311. Follow-ups: native array/map AnyValue attributes (currently JSON-string),
process_tagsas a payload attribute, sidecar 404 fail-closed hardening.