Repository navigation
tracer: keep the trace of a request blocked during startup - #4445
Draft
cataphract wants to merge 1 commit into
Draft
cataphract wants to merge 1 commit into
cataphract wants to merge 1 commit into
Conversation
When AppSec blocks a request while PHP is still in request startup, it calls ddtrace_close_all_spans_and_flush(), which closes every open span and then force-flushes the tracer. The flush has to be forced because ddtrace_flush_tracer() otherwise discards traces while PG(during_request_startup) is set, to avoid reporting requests that never ran any PHP code. With DD_TRACE_AUTO_FLUSH_ENABLED=1 on a web SAPI, closing the root span in the first step already triggers the regular auto-flush. That flush is not forced: it serializes the closed spans, removes them from the tracer and then drops them because the request is still starting up. The forced flush that follows finds nothing left to send, so the trace of the blocked request, which carries the AppSec event, never reaches the agent. Auto-flush is only on by default for the CLI SAPI, where AppSec does not block requests, so this only affects users who enable it explicitly for php-fpm, CGI, Apache and the like. ddtrace_close_all_spans_and_flush() now suppresses the root-span auto-flush while it closes the spans, so that only its own forced flush sends them. Outside request startup this does not change what is sent: before, the auto-flush sent the trace and the forced flush found an empty buffer; now the forced flush sends it. The flag is restored on bailout so that a fatal error in a span close hook cannot leave auto-flush disabled for the rest of the request. Two alternatives were rejected. Passing the force flag into the auto-flush would require threading it through the span closing call chain for a single caller. Checking PG(during_request_startup) before serializing in ddtrace_flush_tracer() would leave the closed spans alive instead of dropping them, which changes behaviour for every request that ends without running PHP code, such as php-fpm status pages or missing scripts. The new test runs a php-cgi child that AppSec blocks in request init, with a mock agent in the parent, and checks that the trace arrives with auto-flush both off and on. Without the fix it fails only in the auto-flush case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 10 Pipeline jobs failed
ℹ️ InfoNo other issues found (see more)🧪 All tests passed 🎯 Code Coverage (details) Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: 1bf0e24 | Docs | View more details | Give us feedback! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note: I'm looking for another strategy for the fix.
Description
When AppSec blocks a request during request startup (RINIT), it calls
ddtrace_close_all_spans_and_flush(). That function closes all open spans, then force-flushes. The flush has to be forced becauseddtrace_flush_tracer()otherwise drops traces whilePG(during_request_startup)is set.With
DD_TRACE_AUTO_FLUSH_ENABLED=1on a web SAPI (php-fpm, CGI, Apache, ...), closing the root span already triggers the regular auto-flush. That flush is not forced, so it serializes the closed spans, removes them, and drops them because the request is still starting up. The forced flush that comes next has nothing left to send. The trace of the blocked request, which carries the AppSec event, never reaches the agent. Auto-flush is on by default only for the CLI SAPI, so only users who turn it on explicitly for a web SAPI are affected.The fix:
ddtrace_close_all_spans_and_flush()turns off the root-span auto-flush while it closes the spans (through a newDDTRACE_G(suppress_auto_flush)flag), so only its own forced flush sends them. The flag is restored if a bailout happens. Outside request startup, what gets sent stays the same: before, the auto-flush sent the trace and the forced flush found an empty buffer; now the forced flush sends it.Alternatives considered:
PG(during_request_startup)before serializing inddtrace_flush_tracer(). Closed spans would then stay alive instead of being dropped, which changes behaviour for every request that ends without running PHP code (php-fpm status page, missing script).Tests
New
appsec/tests/extension/req_abort_in_req_init_auto_flush.phpt. It runs a php-cgi child that AppSec blocks in request init (datadog.appsec.testing_abort_rinit), with a mock agent in the parent. It checks that the trace arrives with auto-flush both off and on. Without the fix, it fails only when auto-flush is on:Runs with the fix:
req_abort_*appsec tests, PHP 7.4 debug: 38 tests, all passed.tests/exttests that useDD_TRACE_AUTO_FLUSH_ENABLEDordd_trace_close_all_spans_and_flush, PHP 8.3 debug, with test-agent and request-replayer: 184 passed, 24 skipped, 0 failed.🤖 Generated with Claude Code