From f27265733334a7bdf81655c4b54e6fa9b7760023 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Andr=C3=A9=20dos=20Santos=20Lopes?= Date: Mon, 27 Jul 2026 17:46:59 +0100 Subject: [PATCH 1/2] fix(sidecar): preserve runtimes during session configuration Update libdatadog so set_session_config no longer drains every runtime in the session for non-fork connections. A sibling FPM worker could otherwise delete an active runtime, causing queued telemetry actions to be discarded after it is recreated without applications. --- Cargo.toml | 3 +++ libdatadog | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 22279e8bab..ed73244658 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,7 +52,10 @@ hyper-util = { version = "0.1.10", features = [ "client", "client-legacy", ] } +prost-build = { version = "0.14.1", default-features = false } +protoc-bin-vendored = { version = "3.0.0", default-features = false } serde = { version = "1.0", default-features = false } +serde_json = { version = "1.0", default-features = false, features = ["alloc"] } tokio = { version = "1.36", default-features = false } tracing = { version = "0.1", default-features = false } diff --git a/libdatadog b/libdatadog index 43156bbe53..f2010b616f 160000 --- a/libdatadog +++ b/libdatadog @@ -1 +1 @@ -Subproject commit 43156bbe53c026fdeeaeb3777cb9d4054507a250 +Subproject commit f2010b616feac15557a2b07e6ffd8c1899c69c63 From c7310d5e751d25d603ecfa0845828c1924078aea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Andr=C3=A9=20dos=20Santos=20Lopes?= Date: Mon, 27 Jul 2026 11:05:16 +0100 Subject: [PATCH 2/2] fix(telemetry): restore trace_api.* background sender metrics The in-process background sender (tracer/coms.c, the default on non-Windows PHP below 8.3 except where the sidecar sender is forced, such as Lambda) counts logical trace-send operations and their final outcomes and reports them as tracers.trace_api.{requests,responses,errors}. They are flushed on a queue id of their own, and the sidecar drops any payload whose queue has no application registered ("No application found for instance ... and queue_id ..."). Nothing registered one, so every flush was discarded. Since b026bfcd99 (2026-06-02) introduced process-global accumulation, each due flush has also drained those counters with atomic_exchange() before handing them over, throwing that interval's counters away. What broke it, and when ----------------------- 91222ad9c ("feat: reduce telemetry sent", #3316, 2025-07-28) updated libdatadog and adapted the telemetry calls. In ddtrace_telemetry_register_services() it dropped ddog_sidecar_telemetry_flushServiceData( &sidecar, ddtrace_sidecar_instance_id, &dd_bgs_queued_id, meta, DDOG_CHARSLICE_C("background_sender-php-service"), DDOG_CHARSLICE_C("none")); which was precisely that queue's application registration, leaving only the registration-only buffer flush and its "FIXME: it seems we must call enqueue_actions (even with an empty list of actions) for things to work properly". The libdatadog rework it pulled in (de42dc229, "feat(sidecar): add telemetry clients expiration", #1077, same day) had just replaced the AppOrQueue buffering - where enqueue_actions on an unknown queue id created an entry and held the actions until a service turned up - with a hard requirement that the application already exist. Before that rework the missing registration would merely have delayed the points; after it, it discards them. Both halves shipped together in 1.11.0 (2025-07-29). Confirmed in the field ---------------------- No version >= 1.11.0 has produced a single trace_api.* point from instrumented_service:background_sender-php-service in any window queried; points exist only for <= 1.10.0. Controls that rule out a bad query or a missing fleet: tracers.spans_created{tracer_version:1.11.0} confirms a large pre-8.3 population on >= 1.11.0 that reports other tracers metrics normally; weekly trace_api volume was flat-to-rising before 1.11.0 and decays monotonically from the week after it shipped, leaving it substantially lower to date. Beware a name collision when checking this: the sidecar's own self telemetry registers the same four metric names in the same namespace and tags them with the PHP tracer version, and it accounts for 93% of all PHP trace_api volume. Discriminate on instrumented_service (background_sender-php-service vs datadog-ipc-helper) or on language_version (a real version vs the literal "sidecar"). The fix ------- - ddtrace_telemetry_bgs_init() generates the queue id once per process from ddtrace_activate_once(), only in the branch that actually starts the sender, so Windows, and PHP 8.3+ with the default sender configuration, never register an application for metrics they do not produce. - dd_bgs_register_application() announces the application from ddtrace_telemetry_finalize(). Not from the post-connect callback, where the old registration lived: dd_activate_once() connects the sidecar before it calls ddtrace_activate_once(), so on the first connection the queue id does not exist yet. A process-global atomic records whether registration was successfully queued on a transport; the post-connect callback (ddtrace_telemetry_register_services) clears it, so the next finalize re-announces the application after a reconnect or when a new ZTS thread establishes a transport. - While unregistered, ddtrace_telemetry_flush_bgs_metrics_if_due() keeps accumulating instead of draining into the void. - The final flush also hands the application back with ddog_sidecar_application_remove(): nothing else in the tracer explicitly removes it. When remote config is enabled, set_universal_service_tags() can attach a subscription to the application, so leaving it behind can pin that subscription for the life of the runtime. Attribution is unchanged: the application carries the same synthetic background_sender-php-service / none pair as before 1.11.0 (these counters describe the sender, not the traced application), so queries written against the old data keep working. tests/ext/telemetry/{simple,broken_pipe}.phpt still filter that service name out of the telemetry they inspect; those branches have been dead since 1.11.0 and are live again. Why the final flush is in MSHUTDOWN and not GSHUTDOWN ----------------------------------------------------- By GSHUTDOWN the process-wide datadog_sidecar_instance_id is already gone, even where the per-thread transport is still alive. MSHUTDOWN calls datadog_sidecar_shutdown(), which NULLs that id - the id both the buffer flush and the application removal use to address the application - and, in thread mode on the master process, also drops the main thread's transport. The engine runs the globals dtor (PHP_GSHUTDOWN(datadog) -> ddtrace_gshutdown()) only after module_shutdown_func has returned, in NTS and ZTS alike. A flush from there can never work in either threading mode; probes printed a live id in MSHUTDOWN and (nil) in GSHUTDOWN on the same thread. The call therefore sits after background-sender teardown has synchronously produced the last counters, and before datadog_sidecar_shutdown() drops the id: a point at which the final metrics exist and can still be sent. One unconditional call site, with no ZTS branch, because the application is keyed by (session id, runtime id, queue id), all three process-wide, so any thread's connection can address it; only the transport is per-thread. Tests ----- Two integration test classes, run on 8.2 release and release-zts. TelemetryBackgroundSenderTests covers the request path: counters produced by request traffic and flushed by a later request. TelemetryBackgroundSenderShutdownTests covers what only happens as a process goes away - the metrics accumulated since the last flush - using a single-request CLI process. Its trace is queued before telemetry finalize, and the sender is synchronously drained later in MSHUTDOWN, exercising the process-exit path. Both tests pass on both variants; in negative-control runs with the MSHUTDOWN flush gated off, the shutdown case fails on both variants and the request-path one keeps passing, which is the whole point of the split. The split is what makes that discrimination possible. Every process reports under the same synthetic service and the sidecar merges same-service telemetry into a single worker, so a payload says nothing about which process produced it: a container that has served requests cannot be used to prove anything about shutdown. The shutdown class therefore owns a container in which no request is ever served. Co-Authored-By: Claude Opus 5 --- appsec/cmake/ddtrace.cmake | 34 ++++--- appsec/tests/integration/build.gradle | 11 ++- .../appsec/php/TelemetryHelpers.groovy | 27 ++++++ ...emetryBackgroundSenderShutdownTests.groovy | 90 +++++++++++++++++++ .../TelemetryBackgroundSenderTests.groovy | 83 +++++++++++++++++ tracer/ddtrace.c | 7 +- tracer/tracer_telemetry.c | 81 +++++++++++++---- tracer/tracer_telemetry.h | 10 ++- 8 files changed, 309 insertions(+), 34 deletions(-) create mode 100644 appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryBackgroundSenderShutdownTests.groovy create mode 100644 appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryBackgroundSenderTests.groovy diff --git a/appsec/cmake/ddtrace.cmake b/appsec/cmake/ddtrace.cmake index 8e0f3092cd..376115a068 100644 --- a/appsec/cmake/ddtrace.cmake +++ b/appsec/cmake/ddtrace.cmake @@ -92,6 +92,8 @@ file(GLOB_RECURSE FILES_DDTRACE CONFIGURE_DEPENDS "${CMAKE_SOURCE_DIR}/../ext/*.c" "${CMAKE_SOURCE_DIR}/../ext/**/*.c" + "${CMAKE_SOURCE_DIR}/../tracer/*.c" + "${CMAKE_SOURCE_DIR}/../tracer/**/*.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/*.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/**/*.c" ) @@ -101,30 +103,38 @@ list(APPEND FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../components/log/log.c" "${CMAKE_SOURCE_DIR}/../components/sapi/sapi.c" "${CMAKE_SOURCE_DIR}/../components/string_view/string_view.c" + "${CMAKE_SOURCE_DIR}/../tracer/vendor/mpack/mpack.c" + "${CMAKE_SOURCE_DIR}/../tracer/vendor/mt19937/mt19937-64.c" ) if (PhpConfig_VERNUM GREATER_EQUAL 80000) - list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/handlers_curl_php7.c" + list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../tracer/handlers_curl_php7.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php7/interceptor.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php7/resolver.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/sandbox/php7/sandbox.c") else() # PHP 7 - list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/handlers_curl.c" - "${CMAKE_SOURCE_DIR}/../ext/hook/uhook_attributes.c" - "${CMAKE_SOURCE_DIR}/../ext/hook/uhook_otel.c" + list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../tracer/handlers_curl.c" + "${CMAKE_SOURCE_DIR}/../tracer/hook/uhook_attributes.c" + "${CMAKE_SOURCE_DIR}/../tracer/hook/uhook_otel.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/interceptor.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/jit_utils/jit_blacklist.c" "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/sandbox/php8/sandbox.c") endif() +if (PhpConfig_VERNUM GREATER_EQUAL 70300) + list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/zend_hrtime.c") +endif() +if (PhpConfig_VERNUM LESS 80000 OR PhpConfig_VERNUM GREATER_EQUAL 80200) + list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/patch_zend_call_known_function.c") +endif() if (PhpConfig_VERNUM LESS 80200) - list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/weakrefs.c") list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver.c") else() # PHP 8.2+ - list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c") + list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../tracer/weakrefs.c" + "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c") endif() if (PhpConfig_VERNUM LESS 80100) - list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/handlers_fiber.c") + list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../tracer/handlers_fiber.c") endif() list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/crashtracking_windows.c") @@ -162,16 +172,20 @@ endif() if(CURL_DEFINITIONS) target_compile_definitions(ddtrace PRIVATE ${CURL_DEFINITIONS}) endif() -target_compile_definitions(ddtrace PRIVATE ZEND_ENABLE_STATIC_TSRMLS_CACHE=1 COMPILE_DL_DDTRACE=1) +target_compile_definitions(ddtrace PRIVATE ZEND_ENABLE_STATIC_TSRMLS_CACHE=1 COMPILE_DL_DDTRACE=1 DDTRACE=1) target_include_directories(ddtrace PRIVATE ${CURL_INCLUDE_DIRS} ${CMAKE_SOURCE_DIR}/.. ${CMAKE_SOURCE_DIR}/../src/dogstatsd ${CMAKE_SOURCE_DIR}/../zend_abstract_interface ${CMAKE_SOURCE_DIR}/../ext - ${CMAKE_SOURCE_DIR}/../ext/vendor - ${CMAKE_SOURCE_DIR}/../ext/vendor/mt19937 + ${CMAKE_SOURCE_DIR}/../tracer + ${CMAKE_SOURCE_DIR}/../tracer/integrations + ${CMAKE_SOURCE_DIR}/../tracer/vendor + ${CMAKE_SOURCE_DIR}/../tracer/vendor/mpack + ${CMAKE_SOURCE_DIR}/../tracer/vendor/mt19937 ${CMAKE_BINARY_DIR}/gen_ddtrace + ${CMAKE_BINARY_DIR}/gen_ddtrace/ext ) add_dependencies(ddtrace ddtrace_exports update_version_h) diff --git a/appsec/tests/integration/build.gradle b/appsec/tests/integration/build.gradle index ecc4d33c60..b86d21e7c7 100644 --- a/appsec/tests/integration/build.gradle +++ b/appsec/tests/integration/build.gradle @@ -343,6 +343,9 @@ def buildTracerTask = { String version, String variant, altBaseTag = null -> inputs: [ dirs: [ '../../../ext', + '../../../tracer', + '../../../components', + '../../../components-rs', '../../../zend_abstract_interface', '../../../libdatadog', '../../../datadog.sym', @@ -375,8 +378,10 @@ def buildTracerCmakeTask = { String version, String variant, altBaseTag = null - inputs: [ dirs: [ '../../../ext', - '../../../zend_abstract_interface', + '../../../tracer', + '../../../components', '../../../components-rs', + '../../../zend_abstract_interface', '../../../libdatadog', '../../../datadog.sym', '../../cmake', @@ -471,8 +476,10 @@ def buildTracerSsiCmakeTask = { String version, String variant -> inputs: [ dirs: [ '../../../ext', - '../../../zend_abstract_interface', + '../../../tracer', + '../../../components', '../../../components-rs', + '../../../zend_abstract_interface', '../../../libdatadog', '../../../datadog.sym', '../../cmake', diff --git a/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/TelemetryHelpers.groovy b/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/TelemetryHelpers.groovy index d9709f6e74..ce3ac9debf 100644 --- a/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/TelemetryHelpers.groovy +++ b/appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/TelemetryHelpers.groovy @@ -41,6 +41,33 @@ class TelemetryHelpers { payloads.collect { type.newInstance([it] as Object[]) } } + /** + * Synthetic service the in-process background sender reports its own metrics under + * (see dd_bgs_register_application() in tracer/tracer_telemetry.c). The counters describe + * the sender, not the application being traced, so they are deliberately not attributed to + * the request's service. + */ + static final String BGS_SERVICE = 'background_sender-php-service' + + /** + * Drains telemetry and returns the {@code generate-metrics} series submitted under the + * given service. + * + *

{@link #filterMessages} only tells the sidecar's own telemetry apart from everything + * else; this keys on the enclosing message's service name, which is what separates the + * background sender's synthetic application ({@link #BGS_SERVICE}) from the traced ones. + */ + static List drainMetricSeries(AppSecContainer container, String service, int timeoutInMs = 500) { + List series = [] + for (msg in container.drainTelemetry(timeoutInMs)) { + if (msg.application?.service_name != service) continue + for (GenerateMetrics metrics in filterMessages([msg], GenerateMetrics, false)) { + series.addAll(metrics.series) + } + } + series + } + static class GenerateMetrics { static names = ['generate-metrics'] List series diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryBackgroundSenderShutdownTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryBackgroundSenderShutdownTests.groovy new file mode 100644 index 0000000000..829128b19e --- /dev/null +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryBackgroundSenderShutdownTests.groovy @@ -0,0 +1,90 @@ +package com.datadog.appsec.php.integration + +import com.datadog.appsec.php.TelemetryHelpers +import com.datadog.appsec.php.TelemetryHelpers.Metric +import com.datadog.appsec.php.docker.AppSecContainer +import com.datadog.appsec.php.docker.FailOnUnmatchedTraces +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.condition.DisabledIf +import org.testcontainers.containers.Container.ExecResult +import org.testcontainers.junit.jupiter.Container +import org.testcontainers.junit.jupiter.Testcontainers + +import static com.datadog.appsec.php.TelemetryHelpers.BGS_SERVICE +import static com.datadog.appsec.php.integration.TestParams.getPhpVersion +import static com.datadog.appsec.php.integration.TestParams.getVariant + +/** + * What the background sender does while its process goes away: submit the counters it has + * accumulated since the last flush from MSHUTDOWN (see ddtrace_mshutdown()), while the sidecar + * can still address the application. + * + * A single-request CLI process isolates the process-exit path: its trace is queued before + * telemetry finalize, and the sender is synchronously drained later in MSHUTDOWN. The FPM + * workers, by contrast, are killed abruptly at the end of a run and never reach MSHUTDOWN. + * + * No request is ever served in this container. Since the background sender's application is + * synthetic and shared by every process (see {@link TelemetryBackgroundSenderTests}), request + * traffic would make any {@code trace_api} point observed here unattributable — that is what + * keeps this apart from the request-path class, which has a container of its own. + */ +@Testcontainers +@DisabledIf('isDisabled') +class TelemetryBackgroundSenderShutdownTests { + static boolean disabled = phpVersion != '8.2' + + @Container + @FailOnUnmatchedTraces + public static final AppSecContainer CONTAINER = + new AppSecContainer( + workVolume: this.name, + baseTag: 'apache2-fpm-php', + phpVersion: phpVersion, + phpVariant: variant, + www: 'base', + ) + + private static final String FLUSH_PROBE_SERVICE = 'bgs_flush_probe' + private static final long METRICS_WAIT_TIMEOUT_MS = 30_000 + private static final long METRICS_POLL_INTERVAL_MS = 500 + + /** + * The process it starts is the only one in the container that can have produced a trace_api + * point: the metrics carry no process identity, so anything already queued for the synthetic + * service would satisfy the assertion below. + * + * The sidecar buffers the points in the telemetry worker for this service/env and emits them + * on its next flush (DD_TELEMETRY_HEARTBEAT_INTERVAL, 10 s here), hence the generous wait. + */ + @Test + void 'metrics accumulated during shutdown are submitted'() { + ExecResult res = CONTAINER.execInContainer('sh', '-c', + "DD_SERVICE=${FLUSH_PROBE_SERVICE} php -r 'usleep(300 * 1000);'; echo status=\$?".toString()) + assert res.stdout.readLines().last() == 'status=0' : "${res.stdout}\n${res.stderr}" + + // consume the trace this generated, or @FailOnUnmatchedTraces trips + assert CONTAINER.nextCapturedTrace() != null + + List series = [] + long deadline = System.currentTimeMillis() + METRICS_WAIT_TIMEOUT_MS + while (!series.any { it.name == 'trace_api.requests' } && + System.currentTimeMillis() < deadline) { + series.addAll(TelemetryHelpers.drainMetricSeries(CONTAINER, BGS_SERVICE, 0)) + if (!series.any { it.name == 'trace_api.requests' }) { + long remaining = deadline - System.currentTimeMillis() + if (remaining > 0) { + Thread.sleep(Math.min(METRICS_POLL_INTERVAL_MS, remaining)) + } + } + } + + Metric requests = series.find { it.name == 'trace_api.requests' } + assert requests != null : "no trace_api.requests for ${BGS_SERVICE}; got ${series*.name}" + assert requests.namespace == 'tracers' + assert requests.points[0][1] >= 1.0 + + Metric responses = series.find { it.name == 'trace_api.responses' } + assert responses != null : 'trace_api.responses not reported at shutdown' + assert 'status_code:2xx' in responses.tags + } +} diff --git a/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryBackgroundSenderTests.groovy b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryBackgroundSenderTests.groovy new file mode 100644 index 0000000000..c56283071d --- /dev/null +++ b/appsec/tests/integration/src/test/groovy/com/datadog/appsec/php/integration/TelemetryBackgroundSenderTests.groovy @@ -0,0 +1,83 @@ +package com.datadog.appsec.php.integration + +import com.datadog.appsec.php.TelemetryHelpers +import com.datadog.appsec.php.TelemetryHelpers.Metric +import com.datadog.appsec.php.docker.AppSecContainer +import com.datadog.appsec.php.docker.FailOnUnmatchedTraces +import groovy.util.logging.Slf4j +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.condition.DisabledIf +import org.testcontainers.junit.jupiter.Container +import org.testcontainers.junit.jupiter.Testcontainers + +import java.net.http.HttpResponse + +import static com.datadog.appsec.php.TelemetryHelpers.BGS_SERVICE +import static com.datadog.appsec.php.integration.TestParams.getPhpVersion +import static com.datadog.appsec.php.integration.TestParams.getVariant + +/** + * The in-process background sender (tracer/coms.c, enabled for every container through + * DD_TRACE_SIDECAR_TRACE_SENDER=0) counts the HTTP exchanges it has with the agent and + * reports them as the trace_api.requests / trace_api.responses telemetry metrics. Those + * counters live on a connection-wide queue id of their own, so the sidecar needs an + * application registered for that queue or it drops the payload with "No application + * found". + * + * That application is deliberately synthetic ({@link TelemetryHelpers#BGS_SERVICE} / env + * {@code none}): the counters describe the sender, not the traced application. It also means + * every process in the container reports them under the same service, and the sidecar merges + * same-service telemetry into a single worker, so the payloads carry nothing that ties them + * back to the process that produced them. + * + * This class covers the ordinary path: counters produced by request traffic and flushed by a + * later request. The paths that only run while a process is going away are in + * {@link TelemetryBackgroundSenderShutdownTests}, which needs a container where no request has + * ever been served — hence a separate class rather than an ordered method here. + * + * Nothing here is version-specific, so a single PHP version is enough, but both threading + * modes are covered. + */ +@Testcontainers +@Slf4j +@DisabledIf('isDisabled') +class TelemetryBackgroundSenderTests { + static boolean disabled = phpVersion != '8.2' + + @Container + @FailOnUnmatchedTraces + public static final AppSecContainer CONTAINER = + new AppSecContainer( + workVolume: this.name, + baseTag: 'apache2-fpm-php', + phpVersion: phpVersion, + phpVariant: variant, + www: 'base', + ) + + @Test + void 'background sender trace_api metrics are reported'() { + List series = [] + for (int i = 0; i < 30 && !series.any { it.name == 'trace_api.requests' }; i++) { + // The counters are only produced once the sender thread has actually talked to the + // agent, and they are only flushed by a *later* request, so keep issuing them. + CONTAINER.traceFromRequest('/hello.php') { HttpResponse resp -> + assert resp.statusCode() == 200 + } + series.addAll(TelemetryHelpers.drainMetricSeries(CONTAINER, BGS_SERVICE)) + } + + Metric requests = series.find { it.name == 'trace_api.requests' } + assert requests != null : "no trace_api.requests for ${BGS_SERVICE}; got ${series*.name}" + assert requests.namespace == 'tracers' + assert requests.type == 'count' + assert requests.points[0][1] >= 1.0 + + Metric responses = series.find { it.name == 'trace_api.responses' } + assert responses != null : 'trace_api.responses metric not received' + assert responses.namespace == 'tracers' + assert responses.type == 'count' + assert responses.points[0][1] >= 1.0 + assert 'status_code:2xx' in responses.tags + } +} diff --git a/tracer/ddtrace.c b/tracer/ddtrace.c index 60181d437f..2192b33406 100644 --- a/tracer/ddtrace.c +++ b/tracer/ddtrace.c @@ -181,6 +181,7 @@ void ddtrace_activate_once(void) { // Set the default to 5000 so that BGS does not flush too often. The sidecar can flush more often, but the BGS is per process. Keep it higher to avoid too much load on the agent. zai_config_change_default_ini(DATADOG_CONFIG_DD_TRACE_AGENT_FLUSH_INTERVAL, (zai_str) ZAI_STR_FROM_CSTR("5000")); } + ddtrace_telemetry_bgs_init(); ddtrace_coms_minit(get_global_DD_TRACE_AGENT_STACK_INITIAL_SIZE(), get_global_DD_TRACE_AGENT_MAX_PAYLOAD_SIZE(), get_global_DD_TRACE_AGENT_STACK_BACKLOG()); @@ -241,10 +242,6 @@ void ddtrace_gshutdown(zend_datadog_globals *datadog_globals) { if (datadog_globals->ddtrace.agent_config_reader) { ddog_agent_remote_config_reader_drop(datadog_globals->ddtrace.agent_config_reader); } - if (datadog_globals->sidecar) { - // Drain any accumulated background-sender metrics before the transport goes away. - ddtrace_telemetry_flush_bgs_metrics_final(datadog_globals); - } } @@ -400,6 +397,8 @@ void ddtrace_mshutdown() { ddog_sidecar_flush(&DATADOG_G(sidecar), (ddog_SidecarFlushOptions){.traces_and_stats = true, .telemetry = true}); } + ddtrace_telemetry_flush_bgs_metrics_final(); + ddtrace_engine_hooks_mshutdown(); ddtrace_shutdown_proxy_info_map(); diff --git a/tracer/tracer_telemetry.c b/tracer/tracer_telemetry.c index 097b755b7e..10a79c563e 100644 --- a/tracer/tracer_telemetry.c +++ b/tracer/tracer_telemetry.c @@ -19,7 +19,19 @@ ZEND_EXTERN_MODULE_GLOBALS(datadog); zend_long dd_composer_hook_id; -ddog_QueueId dd_bgs_queued_id; + +static ddog_QueueId dd_bgs_queue_id; + +// Whether the sidecar has been told about the BGS application. Process-wide, like the +// application itself: it is keyed by (session id, runtime id, queue id), all three of which are +// process-wide (ext/sidecar.c), so a single announcement over any thread's connection is visible +// to every other thread's flush. A reconnect clears it (ddtrace_telemetry_register_services) and +// the next finalize announces it again, over whichever connection gets there first. +// uint64_t rather than bool because the Windows atomics polyfill only has 64-bit intrinsics +// (components/atomic_win32_polyfill.h). +static _Atomic(uint64_t) dd_bgs_application_registered = 0; + +static void dd_bgs_register_application(void); const char *ddtrace_telemetry_redact_file(const char *file) { #ifdef _WIN32 @@ -109,6 +121,7 @@ void ddtrace_telemetry_finalize(void) { ddog_sidecar_telemetry_add_span_metric_point_buffer(buffer, DDOG_CHARSLICE_C("context_header_style.malformed"), DDTRACE_G(baggage_malformed_count), DDOG_CHARSLICE_C("header_style:baggage")); // Flush any accumulated BGS (background sender) metrics if enough time has passed. + dd_bgs_register_application(); ddtrace_telemetry_flush_bgs_metrics_if_due(DATADOG_GLOBALS_PTR()); } @@ -128,18 +141,41 @@ void ddtrace_telemetry_rshutdown(void) { } void ddtrace_telemetry_register_services(ddog_SidecarTransport **sidecar) { - if (!dd_bgs_queued_id) { - dd_bgs_queued_id = ddog_sidecar_queueId_generate(); - } - ddog_sidecar_telemetry_register_metric(sidecar, DDOG_CHARSLICE_C("trace_api.requests"), DDOG_METRIC_TYPE_COUNT, DDOG_METRIC_NAMESPACE_TRACERS); ddog_sidecar_telemetry_register_metric(sidecar, DDOG_CHARSLICE_C("trace_api.responses"), DDOG_METRIC_TYPE_COUNT, DDOG_METRIC_NAMESPACE_TRACERS); ddog_sidecar_telemetry_register_metric(sidecar, DDOG_CHARSLICE_C("trace_api.errors"), DDOG_METRIC_TYPE_COUNT, DDOG_METRIC_NAMESPACE_TRACERS); - // FIXME: it seems we must call "enqueue_actions" (even with an empty list of actions) for things to work properly - ddog_SidecarActionsBuffer *buffer = ddog_sidecar_telemetry_buffer_alloc(); - datadog_ffi_try("Failed flushing background sender telemetry buffer", - ddog_sidecar_telemetry_buffer_flush(sidecar, datadog_sidecar_instance_id, &dd_bgs_queued_id, buffer)); + // this function is in fact a sidecar post-connect callback, + // so the application has been forgotten + atomic_store(&dd_bgs_application_registered, 0); +} + +// must be called before dd_bgs_register_application() +void ddtrace_telemetry_bgs_init(void) { + dd_bgs_queue_id = ddog_sidecar_queueId_generate(); +} + +// Gives the BGS queue an application, without which the sidecar has no service/env to attribute +// its metrics to and drops them ("No application found"). The service/env pair is synthetic and +// fixed: these counters describe the sender, not the application being traced, and this is the +// pair they have always been reported under (up to 1.10.0), so existing queries keep working. +static void dd_bgs_register_application(void) { + if (!dd_bgs_queue_id || !DATADOG_G(sidecar) || atomic_load(&dd_bgs_application_registered)) { + return; + } + + // Only consider it registered if the sidecar took it. Flushing against an application that + // does not exist is not merely a no-op: the flush drains the counters with atomic_exchange() + // before handing them over, so every subsequent interval would zero them and throw them away. + atomic_store(&dd_bgs_application_registered, + datadog_ffi_try("Failed registering background sender application", + ddog_sidecar_set_universal_service_tags(&DATADOG_G(sidecar), datadog_sidecar_instance_id, &dd_bgs_queue_id, + DDOG_CHARSLICE_C("background_sender-php-service"), + DDOG_CHARSLICE_C("none"), + DDOG_CHARSLICE_C(""), // no app version, as before + &DATADOG_G(active_global_tags), + DDOG_DYNAMIC_INSTRUMENTATION_CONFIG_STATE_DISABLED, + UINT64_MAX))); } void ddtrace_telemetry_notify_integration(const char *name, size_t name_len) { @@ -219,6 +255,10 @@ void ddtrace_telemetry_flush_bgs_metrics_if_due(zend_datadog_globals *datadog_gl if (!datadog_globals->sidecar || !get_global_DD_INSTRUMENTATION_TELEMETRY_ENABLED()) { return; } + if (!atomic_load(&dd_bgs_application_registered)) { + // The sidecar would have nowhere to file these metrics; keep accumulating instead. + return; + } // Rate-limit: flush at most once per agent flush interval. uint64_t now_ns = ddtrace_nanoseconds_realtime(); @@ -267,13 +307,24 @@ void ddtrace_telemetry_flush_bgs_metrics_if_due(zend_datadog_globals *datadog_gl } datadog_ffi_try("Failed flushing background sender metrics", - ddog_sidecar_telemetry_buffer_flush(&datadog_globals->sidecar, datadog_sidecar_instance_id, &dd_bgs_queued_id, buffer)); + ddog_sidecar_telemetry_buffer_flush(&datadog_globals->sidecar, datadog_sidecar_instance_id, &dd_bgs_queue_id, buffer)); } -void ddtrace_telemetry_flush_bgs_metrics_final(zend_datadog_globals *datadog_globals) { - // Bypass the time gate so any remaining metrics are sent before the transport - // is dropped in GSHUTDOWN. Setting last_flush_ns to 0 makes the time check in - // _if_due always pass; the CAS inside still prevents a concurrent double-flush. +void ddtrace_telemetry_flush_bgs_metrics_final(void) { + if (!atomic_load(&dd_bgs_application_registered) || !DATADOG_G(sidecar) || !datadog_sidecar_instance_id) { + return; + } + + // Bypass the time gate so any remaining metrics are sent while there is still a connection to + // send them on. Setting last_flush_ns to 0 makes the time check in _if_due always pass; the + // CAS inside still prevents a concurrent double-flush. atomic_store(&bgs_metrics_last_flush_ns, 0); - ddtrace_telemetry_flush_bgs_metrics_if_due(datadog_globals); + ddtrace_telemetry_flush_bgs_metrics_if_due(DATADOG_GLOBALS_PTR()); + + if (!atomic_exchange(&dd_bgs_application_registered, 0)) { + return; + } + + datadog_ffi_try("Failed removing background sender application", + ddog_sidecar_application_remove(&DATADOG_G(sidecar), datadog_sidecar_instance_id, &dd_bgs_queue_id)); } diff --git a/tracer/tracer_telemetry.h b/tracer/tracer_telemetry.h index 3ad266ad1f..95594d391e 100644 --- a/tracer/tracer_telemetry.h +++ b/tracer/tracer_telemetry.h @@ -12,14 +12,18 @@ void ddtrace_telemetry_notify_integration(const char *name, size_t name_len); void ddtrace_telemetry_notify_integration_version(const char *name, size_t name_len, const char *version, size_t version_len); void ddtrace_telemetry_inc_spans_created(ddtrace_span_data *span); +// Generates the queue id the BGS metrics are reported on. Call once per process, before any +// request thread runs, and only when the background sender is in use. +void ddtrace_telemetry_bgs_init(void); // Called by the background sender thread (coms.c) to accumulate metrics atomically. // Never touches the sidecar; the request thread flushes via ddtrace_telemetry_flush_bgs_metrics_if_due(). void ddtrace_telemetry_send_trace_api_metrics(trace_api_metrics metrics); // Called from datadog_telemetry_finalize() to flush accumulated BGS metrics through // the current thread's sidecar connection, at most once per flush interval. void ddtrace_telemetry_flush_bgs_metrics_if_due(zend_datadog_globals *datadog_globals); -// Force-flush accumulated BGS metrics regardless of the time gate. Call immediately -// before dropping the per-thread transport in GSHUTDOWN so no data is lost. -void ddtrace_telemetry_flush_bgs_metrics_final(zend_datadog_globals *datadog_globals); +// Force-flush accumulated BGS metrics regardless of the time gate and release the application +// they are reported under. Call once per process from MSHUTDOWN, before the sidecar's instance +// id is dropped. +void ddtrace_telemetry_flush_bgs_metrics_final(void); #endif // DD_TRACER_TELEMETRY_H