Skip to content

Commit 3b3d1a3

Browse files
authored
Merge branch 'master' into glopes/add-system-tests-scenario
2 parents 898ca6a + a732d12 commit 3b3d1a3

10 files changed

Lines changed: 313 additions & 35 deletions

File tree

Cargo.toml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,10 @@ hyper-util = { version = "0.1.10", features = [
5252
"client",
5353
"client-legacy",
5454
] }
55+
prost-build = { version = "0.14.1", default-features = false }
56+
protoc-bin-vendored = { version = "3.0.0", default-features = false }
5557
serde = { version = "1.0", default-features = false }
58+
serde_json = { version = "1.0", default-features = false, features = ["alloc"] }
5659
tokio = { version = "1.36", default-features = false }
5760
tracing = { version = "0.1", default-features = false }
5861

appsec/cmake/ddtrace.cmake

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,8 @@ file(GLOB_RECURSE FILES_DDTRACE
9292
CONFIGURE_DEPENDS
9393
"${CMAKE_SOURCE_DIR}/../ext/*.c"
9494
"${CMAKE_SOURCE_DIR}/../ext/**/*.c"
95+
"${CMAKE_SOURCE_DIR}/../tracer/*.c"
96+
"${CMAKE_SOURCE_DIR}/../tracer/**/*.c"
9597
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/*.c"
9698
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/**/*.c"
9799
)
@@ -101,30 +103,38 @@ list(APPEND FILES_DDTRACE
101103
"${CMAKE_SOURCE_DIR}/../components/log/log.c"
102104
"${CMAKE_SOURCE_DIR}/../components/sapi/sapi.c"
103105
"${CMAKE_SOURCE_DIR}/../components/string_view/string_view.c"
106+
"${CMAKE_SOURCE_DIR}/../tracer/vendor/mpack/mpack.c"
107+
"${CMAKE_SOURCE_DIR}/../tracer/vendor/mt19937/mt19937-64.c"
104108
)
105109
if (PhpConfig_VERNUM GREATER_EQUAL 80000)
106-
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/handlers_curl_php7.c"
110+
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../tracer/handlers_curl_php7.c"
107111
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php7/interceptor.c"
108112
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php7/resolver.c"
109113
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/sandbox/php7/sandbox.c")
110114
else() # PHP 7
111-
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/handlers_curl.c"
112-
"${CMAKE_SOURCE_DIR}/../ext/hook/uhook_attributes.c"
113-
"${CMAKE_SOURCE_DIR}/../ext/hook/uhook_otel.c"
115+
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../tracer/handlers_curl.c"
116+
"${CMAKE_SOURCE_DIR}/../tracer/hook/uhook_attributes.c"
117+
"${CMAKE_SOURCE_DIR}/../tracer/hook/uhook_otel.c"
114118
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/interceptor.c"
115119
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver.c"
116120
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c"
117121
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/jit_utils/jit_blacklist.c"
118122
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/sandbox/php8/sandbox.c")
119123
endif()
124+
if (PhpConfig_VERNUM GREATER_EQUAL 70300)
125+
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/zend_hrtime.c")
126+
endif()
127+
if (PhpConfig_VERNUM LESS 80000 OR PhpConfig_VERNUM GREATER_EQUAL 80200)
128+
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/patch_zend_call_known_function.c")
129+
endif()
120130
if (PhpConfig_VERNUM LESS 80200)
121-
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/weakrefs.c")
122131
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver.c")
123132
else() # PHP 8.2+
124-
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c")
133+
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../tracer/weakrefs.c"
134+
"${CMAKE_SOURCE_DIR}/../zend_abstract_interface/interceptor/php8/resolver_pre-8_2.c")
125135
endif()
126136
if (PhpConfig_VERNUM LESS 80100)
127-
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/handlers_fiber.c")
137+
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../tracer/handlers_fiber.c")
128138
endif()
129139
list(REMOVE_ITEM FILES_DDTRACE "${CMAKE_SOURCE_DIR}/../ext/crashtracking_windows.c")
130140

@@ -162,16 +172,20 @@ endif()
162172
if(CURL_DEFINITIONS)
163173
target_compile_definitions(ddtrace PRIVATE ${CURL_DEFINITIONS})
164174
endif()
165-
target_compile_definitions(ddtrace PRIVATE ZEND_ENABLE_STATIC_TSRMLS_CACHE=1 COMPILE_DL_DDTRACE=1)
175+
target_compile_definitions(ddtrace PRIVATE ZEND_ENABLE_STATIC_TSRMLS_CACHE=1 COMPILE_DL_DDTRACE=1 DDTRACE=1)
166176
target_include_directories(ddtrace PRIVATE
167177
${CURL_INCLUDE_DIRS}
168178
${CMAKE_SOURCE_DIR}/..
169179
${CMAKE_SOURCE_DIR}/../src/dogstatsd
170180
${CMAKE_SOURCE_DIR}/../zend_abstract_interface
171181
${CMAKE_SOURCE_DIR}/../ext
172-
${CMAKE_SOURCE_DIR}/../ext/vendor
173-
${CMAKE_SOURCE_DIR}/../ext/vendor/mt19937
182+
${CMAKE_SOURCE_DIR}/../tracer
183+
${CMAKE_SOURCE_DIR}/../tracer/integrations
184+
${CMAKE_SOURCE_DIR}/../tracer/vendor
185+
${CMAKE_SOURCE_DIR}/../tracer/vendor/mpack
186+
${CMAKE_SOURCE_DIR}/../tracer/vendor/mt19937
174187
${CMAKE_BINARY_DIR}/gen_ddtrace
188+
${CMAKE_BINARY_DIR}/gen_ddtrace/ext
175189
)
176190
add_dependencies(ddtrace ddtrace_exports update_version_h)
177191

appsec/tests/integration/build.gradle

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,9 @@ def buildTracerTask = { String version, String variant, altBaseTag = null ->
343343
inputs: [
344344
dirs: [
345345
'../../../ext',
346+
'../../../tracer',
347+
'../../../components',
348+
'../../../components-rs',
346349
'../../../zend_abstract_interface',
347350
'../../../libdatadog',
348351
'../../../datadog.sym',
@@ -375,8 +378,10 @@ def buildTracerCmakeTask = { String version, String variant, altBaseTag = null -
375378
inputs: [
376379
dirs: [
377380
'../../../ext',
378-
'../../../zend_abstract_interface',
381+
'../../../tracer',
382+
'../../../components',
379383
'../../../components-rs',
384+
'../../../zend_abstract_interface',
380385
'../../../libdatadog',
381386
'../../../datadog.sym',
382387
'../../cmake',
@@ -471,8 +476,10 @@ def buildTracerSsiCmakeTask = { String version, String variant ->
471476
inputs: [
472477
dirs: [
473478
'../../../ext',
474-
'../../../zend_abstract_interface',
479+
'../../../tracer',
480+
'../../../components',
475481
'../../../components-rs',
482+
'../../../zend_abstract_interface',
476483
'../../../libdatadog',
477484
'../../../datadog.sym',
478485
'../../cmake',

appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/TelemetryHelpers.groovy

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,33 @@ class TelemetryHelpers {
4141
payloads.collect { type.newInstance([it] as Object[]) }
4242
}
4343

44+
/**
45+
* Synthetic service the in-process background sender reports its own metrics under
46+
* (see dd_bgs_register_application() in tracer/tracer_telemetry.c). The counters describe
47+
* the sender, not the application being traced, so they are deliberately not attributed to
48+
* the request's service.
49+
*/
50+
static final String BGS_SERVICE = 'background_sender-php-service'
51+
52+
/**
53+
* Drains telemetry and returns the {@code generate-metrics} series submitted under the
54+
* given service.
55+
*
56+
* <p>{@link #filterMessages} only tells the sidecar's own telemetry apart from everything
57+
* else; this keys on the enclosing message's service name, which is what separates the
58+
* background sender's synthetic application ({@link #BGS_SERVICE}) from the traced ones.
59+
*/
60+
static List<Metric> drainMetricSeries(AppSecContainer container, String service, int timeoutInMs = 500) {
61+
List<Metric> series = []
62+
for (msg in container.drainTelemetry(timeoutInMs)) {
63+
if (msg.application?.service_name != service) continue
64+
for (GenerateMetrics metrics in filterMessages([msg], GenerateMetrics, false)) {
65+
series.addAll(metrics.series)
66+
}
67+
}
68+
series
69+
}
70+
4471
static class GenerateMetrics {
4572
static names = ['generate-metrics']
4673
List<Metric> series
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
package com.datadog.appsec.php.integration
2+
3+
import com.datadog.appsec.php.TelemetryHelpers
4+
import com.datadog.appsec.php.TelemetryHelpers.Metric
5+
import com.datadog.appsec.php.docker.AppSecContainer
6+
import com.datadog.appsec.php.docker.FailOnUnmatchedTraces
7+
import org.junit.jupiter.api.Test
8+
import org.junit.jupiter.api.condition.DisabledIf
9+
import org.testcontainers.containers.Container.ExecResult
10+
import org.testcontainers.junit.jupiter.Container
11+
import org.testcontainers.junit.jupiter.Testcontainers
12+
13+
import static com.datadog.appsec.php.TelemetryHelpers.BGS_SERVICE
14+
import static com.datadog.appsec.php.integration.TestParams.getPhpVersion
15+
import static com.datadog.appsec.php.integration.TestParams.getVariant
16+
17+
/**
18+
* What the background sender does while its process goes away: submit the counters it has
19+
* accumulated since the last flush from MSHUTDOWN (see ddtrace_mshutdown()), while the sidecar
20+
* can still address the application.
21+
*
22+
* A single-request CLI process isolates the process-exit path: its trace is queued before
23+
* telemetry finalize, and the sender is synchronously drained later in MSHUTDOWN. The FPM
24+
* workers, by contrast, are killed abruptly at the end of a run and never reach MSHUTDOWN.
25+
*
26+
* No request is ever served in this container. Since the background sender's application is
27+
* synthetic and shared by every process (see {@link TelemetryBackgroundSenderTests}), request
28+
* traffic would make any {@code trace_api} point observed here unattributable — that is what
29+
* keeps this apart from the request-path class, which has a container of its own.
30+
*/
31+
@Testcontainers
32+
@DisabledIf('isDisabled')
33+
class TelemetryBackgroundSenderShutdownTests {
34+
static boolean disabled = phpVersion != '8.2'
35+
36+
@Container
37+
@FailOnUnmatchedTraces
38+
public static final AppSecContainer CONTAINER =
39+
new AppSecContainer(
40+
workVolume: this.name,
41+
baseTag: 'apache2-fpm-php',
42+
phpVersion: phpVersion,
43+
phpVariant: variant,
44+
www: 'base',
45+
)
46+
47+
private static final String FLUSH_PROBE_SERVICE = 'bgs_flush_probe'
48+
private static final long METRICS_WAIT_TIMEOUT_MS = 30_000
49+
private static final long METRICS_POLL_INTERVAL_MS = 500
50+
51+
/**
52+
* The process it starts is the only one in the container that can have produced a trace_api
53+
* point: the metrics carry no process identity, so anything already queued for the synthetic
54+
* service would satisfy the assertion below.
55+
*
56+
* The sidecar buffers the points in the telemetry worker for this service/env and emits them
57+
* on its next flush (DD_TELEMETRY_HEARTBEAT_INTERVAL, 10 s here), hence the generous wait.
58+
*/
59+
@Test
60+
void 'metrics accumulated during shutdown are submitted'() {
61+
ExecResult res = CONTAINER.execInContainer('sh', '-c',
62+
"DD_SERVICE=${FLUSH_PROBE_SERVICE} php -r 'usleep(300 * 1000);'; echo status=\$?".toString())
63+
assert res.stdout.readLines().last() == 'status=0' : "${res.stdout}\n${res.stderr}"
64+
65+
// consume the trace this generated, or @FailOnUnmatchedTraces trips
66+
assert CONTAINER.nextCapturedTrace() != null
67+
68+
List<Metric> series = []
69+
long deadline = System.currentTimeMillis() + METRICS_WAIT_TIMEOUT_MS
70+
while (!series.any { it.name == 'trace_api.requests' } &&
71+
System.currentTimeMillis() < deadline) {
72+
series.addAll(TelemetryHelpers.drainMetricSeries(CONTAINER, BGS_SERVICE, 0))
73+
if (!series.any { it.name == 'trace_api.requests' }) {
74+
long remaining = deadline - System.currentTimeMillis()
75+
if (remaining > 0) {
76+
Thread.sleep(Math.min(METRICS_POLL_INTERVAL_MS, remaining))
77+
}
78+
}
79+
}
80+
81+
Metric requests = series.find { it.name == 'trace_api.requests' }
82+
assert requests != null : "no trace_api.requests for ${BGS_SERVICE}; got ${series*.name}"
83+
assert requests.namespace == 'tracers'
84+
assert requests.points[0][1] >= 1.0
85+
86+
Metric responses = series.find { it.name == 'trace_api.responses' }
87+
assert responses != null : 'trace_api.responses not reported at shutdown'
88+
assert 'status_code:2xx' in responses.tags
89+
}
90+
}
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
package com.datadog.appsec.php.integration
2+
3+
import com.datadog.appsec.php.TelemetryHelpers
4+
import com.datadog.appsec.php.TelemetryHelpers.Metric
5+
import com.datadog.appsec.php.docker.AppSecContainer
6+
import com.datadog.appsec.php.docker.FailOnUnmatchedTraces
7+
import groovy.util.logging.Slf4j
8+
import org.junit.jupiter.api.Test
9+
import org.junit.jupiter.api.condition.DisabledIf
10+
import org.testcontainers.junit.jupiter.Container
11+
import org.testcontainers.junit.jupiter.Testcontainers
12+
13+
import java.net.http.HttpResponse
14+
15+
import static com.datadog.appsec.php.TelemetryHelpers.BGS_SERVICE
16+
import static com.datadog.appsec.php.integration.TestParams.getPhpVersion
17+
import static com.datadog.appsec.php.integration.TestParams.getVariant
18+
19+
/**
20+
* The in-process background sender (tracer/coms.c, enabled for every container through
21+
* DD_TRACE_SIDECAR_TRACE_SENDER=0) counts the HTTP exchanges it has with the agent and
22+
* reports them as the trace_api.requests / trace_api.responses telemetry metrics. Those
23+
* counters live on a connection-wide queue id of their own, so the sidecar needs an
24+
* application registered for that queue or it drops the payload with "No application
25+
* found".
26+
*
27+
* That application is deliberately synthetic ({@link TelemetryHelpers#BGS_SERVICE} / env
28+
* {@code none}): the counters describe the sender, not the traced application. It also means
29+
* every process in the container reports them under the same service, and the sidecar merges
30+
* same-service telemetry into a single worker, so the payloads carry nothing that ties them
31+
* back to the process that produced them.
32+
*
33+
* This class covers the ordinary path: counters produced by request traffic and flushed by a
34+
* later request. The paths that only run while a process is going away are in
35+
* {@link TelemetryBackgroundSenderShutdownTests}, which needs a container where no request has
36+
* ever been served — hence a separate class rather than an ordered method here.
37+
*
38+
* Nothing here is version-specific, so a single PHP version is enough, but both threading
39+
* modes are covered.
40+
*/
41+
@Testcontainers
42+
@Slf4j
43+
@DisabledIf('isDisabled')
44+
class TelemetryBackgroundSenderTests {
45+
static boolean disabled = phpVersion != '8.2'
46+
47+
@Container
48+
@FailOnUnmatchedTraces
49+
public static final AppSecContainer CONTAINER =
50+
new AppSecContainer(
51+
workVolume: this.name,
52+
baseTag: 'apache2-fpm-php',
53+
phpVersion: phpVersion,
54+
phpVariant: variant,
55+
www: 'base',
56+
)
57+
58+
@Test
59+
void 'background sender trace_api metrics are reported'() {
60+
List<Metric> series = []
61+
for (int i = 0; i < 30 && !series.any { it.name == 'trace_api.requests' }; i++) {
62+
// The counters are only produced once the sender thread has actually talked to the
63+
// agent, and they are only flushed by a *later* request, so keep issuing them.
64+
CONTAINER.traceFromRequest('/hello.php') { HttpResponse<InputStream> resp ->
65+
assert resp.statusCode() == 200
66+
}
67+
series.addAll(TelemetryHelpers.drainMetricSeries(CONTAINER, BGS_SERVICE))
68+
}
69+
70+
Metric requests = series.find { it.name == 'trace_api.requests' }
71+
assert requests != null : "no trace_api.requests for ${BGS_SERVICE}; got ${series*.name}"
72+
assert requests.namespace == 'tracers'
73+
assert requests.type == 'count'
74+
assert requests.points[0][1] >= 1.0
75+
76+
Metric responses = series.find { it.name == 'trace_api.responses' }
77+
assert responses != null : 'trace_api.responses metric not received'
78+
assert responses.namespace == 'tracers'
79+
assert responses.type == 'count'
80+
assert responses.points[0][1] >= 1.0
81+
assert 'status_code:2xx' in responses.tags
82+
}
83+
}

libdatadog

tracer/ddtrace.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ void ddtrace_activate_once(void) {
181181
// 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.
182182
zai_config_change_default_ini(DATADOG_CONFIG_DD_TRACE_AGENT_FLUSH_INTERVAL, (zai_str) ZAI_STR_FROM_CSTR("5000"));
183183
}
184+
ddtrace_telemetry_bgs_init();
184185
ddtrace_coms_minit(get_global_DD_TRACE_AGENT_STACK_INITIAL_SIZE(),
185186
get_global_DD_TRACE_AGENT_MAX_PAYLOAD_SIZE(),
186187
get_global_DD_TRACE_AGENT_STACK_BACKLOG());
@@ -241,10 +242,6 @@ void ddtrace_gshutdown(zend_datadog_globals *datadog_globals) {
241242
if (datadog_globals->ddtrace.agent_config_reader) {
242243
ddog_agent_remote_config_reader_drop(datadog_globals->ddtrace.agent_config_reader);
243244
}
244-
if (datadog_globals->sidecar) {
245-
// Drain any accumulated background-sender metrics before the transport goes away.
246-
ddtrace_telemetry_flush_bgs_metrics_final(datadog_globals);
247-
}
248245
}
249246

250247

@@ -400,6 +397,8 @@ void ddtrace_mshutdown() {
400397
ddog_sidecar_flush(&DATADOG_G(sidecar), (ddog_SidecarFlushOptions){.traces_and_stats = true, .telemetry = true});
401398
}
402399

400+
ddtrace_telemetry_flush_bgs_metrics_final();
401+
403402
ddtrace_engine_hooks_mshutdown();
404403
ddtrace_shutdown_proxy_info_map();
405404

0 commit comments

Comments
 (0)