Repository navigation
Enable DSM tests for PHP - #8202
arjunguhaswe wants to merge 9 commits into
Conversation
|
|
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 2 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 9217985 | Docs | View more details | Give us feedback! |
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. |
![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: aeb08123ef
ℹ️ 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".
| ENV DD_TRACE_GENERATE_ROOT_SPAN=1 | ||
| ENV DD_TRACE_AGENT_FLUSH_AFTER_N_REQUESTS=0 | ||
| ENV DD_TRACE_HEADER_TAGS=user-agent | ||
| ENV DD_DATA_STREAMS_ENABLED=true |
There was a problem hiding this comment.
When either PHP weblog runs in DEFAULT, AppSec, telemetry, or any other scenario, this image-level setting still enables DSM and therefore changes tracer behavior outside the scenario being activated here. Since all new DSM tests are restricted to @scenarios.integrations, set DD_DATA_STREAMS_ENABLED in Integrations.weblog_env instead; the same issue also applies to php-fpm.base.Dockerfile.
AGENTS.md reference: AGENTS.md:L34-L36
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. 2cc141f moves DD_DATA_STREAMS_ENABLED from the PHP base Dockerfiles to Integrations.weblog_env, so DSM is on only in INTEGRATIONS. The other tracers with DSM tests already set it in their weblog Dockerfiles, so this changes nothing for them. Locally, the DSM tests still pass on php-fpm-8.2 and apache-mod-8.0 with the variable coming from the scenario.
| $channel->basic_consume($queue, '', false, true, false, false, function () use ($queue, &$pending) { | ||
| unset($pending[$queue]); | ||
| }); |
There was a problem hiding this comment.
When the reusable RabbitMQ container has a queued message from an earlier or timed-out request, this callback clears $pending for that queue without checking the payload, so the endpoint can return ok without consuming the message it just published and the DSM assertion can be satisfied by stale propagation context. This is especially likely for the topic case because queue 3 matches two routing keys while only one delivery is required; publish a unique request payload and clear a queue only after receiving that payload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2cc141f. Each request publishes a unique body, and a queue only counts as consumed once that body arrives, so messages left by an earlier request are consumed and skipped. I checked this locally by seeding a message with no DSM context into the direct queue and the three fanout queues, then calling the endpoints. Both returned ok, the queues ended empty, and the agent received the leftovers' checkpoints (parent 0) alongside the expected ones parented to the publish.
cbeauchesne
left a comment
cbeauchesne
left a comment
There was a problem hiding this comment.
You'll also need to remove the branch selector, and rebase the PR.
6e72a51 to
3782005
Compare
arjunguhaswe
changed the title
217b74a to
55e1e66
Compare
Add a /dsm endpoint to the plain PHP weblog (php-fpm and apache-mod): Kafka through php-rdkafka, RabbitMQ direct, topic and fanout exchanges through php-amqplib, and /dsm/inject and /dsm/extract. Install the clients in the base images, enable DSM there, and enable the tests from 1.27.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dd-trace-php#4380 now tags Kafka checkpoints with the cluster id, so PHP runs test_dsm_kafka, and test_dsm_kafka_without_cluster_id is irrelevant as for the other tracers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Set DD_DATA_STREAMS_ENABLED in Integrations.weblog_env instead of the PHP base images, so DSM is on only in the scenario that tests it. The other tracers with DSM tests already enable it in their weblogs. The RabbitMQ endpoints now wait for the message they published, and skip messages an earlier request left in a queue. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The apache-mod base images now download php-rdkafka's release tarball from PECL, as dd-trace-php's CI images do, retry the download, and check its sha256 before building it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Resolve digests for the PHP base images that build_base_images published, and restore datadog/agent:latest, which a local binaries/agent-image pin had replaced in mirror_images.yaml. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
52f9d98 to
55b63ca
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
Enables the DSM tests for PHP, for the DSM support added by DataDog/dd-trace-php#4380 (merged; ships in 1.27.0): Kafka (with
kafka_cluster_id), RabbitMQ direct, topic and fanout exchanges, and base64 context injection and extraction.How
weblogs/plain/dsm.phpadds the/dsm,/dsm/injectand/dsm/extractendpoints to every php-fpm and apache-mod weblog. Kafka goes through php-rdkafka and RabbitMQ through php-amqplib. Each request waits for its own message before answeringok.Test_DsmKafka's 5 s default timeout. Its checkpoints still carry the group.DD_DATA_STREAMS_ENABLED=trueis set inIntegrations.weblog_env. The other DSM tracers already set it in their Dockerfiles.missing_feature.Testing
1.27.0+dev.aad0ff19, with dd-trace-php#4380 merged), on php-fpm-8.2: all 6 enabled DSM tests pass.ab5998e35, on php-fpm-8.2 and apache-mod-8.0: the same 6 pass, and the full INTEGRATIONS scenario has no failures.🤖 Generated with Claude Code