Skip to content

Commit ef19035

Browse files
cataphractclaude
andcommitted
appsec: assert app-extended-heartbeat re-emits full telemetry state
Updates libdatadog to a version that includes dependencies and integrations in app-extended-heartbeat. The previous behaviour omitted those fields and caused a duplicate app-integrations-change / app-dependencies-loaded on the next regular flush. Adds an integration test (TelemetryExtendedHeartbeatTests) that drives phpredis and exec via custom_integrations.php and asserts: 1. They appear in a regular app-started/app-integrations-change. 2. The next app-extended-heartbeat re-emits configuration, dependencies, and integrations (with phpredis present). 3. Loading a fresh integration afterwards produces an app-integrations-change containing only the new integration — phpredis is not re-emitted, confirming the leak is fixed. The new test runs in its own container so DD_TELEMETRY_EXTENDED_HEARTBEAT_INTERVAL=15 applies before the worker spawns. The existing TelemetryTests no longer sets that env var, so its 24h default no longer interferes with tests that are not about the extended heartbeat (this addresses the intermittent 'telemetry reflects the loading of a new integration' failure caused by the heartbeat re-queueing data into the next regular flush). Folds two helpers into TelemetryHelpers: - filterMessages(...) gains a userAppOnly flag (default true) that skips the sidecar's own datadog-ipc-helper telemetry worker. - WithExtendedHeartbeat / waitForExtendedHeartbeat mirror the existing WithIntegrations / waitForIntegrations pattern. custom_integrations.php gains an "fs" branch (fopen on /dev/null) so tests can trigger the filesystem deferred-loading hook. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 29c8781 commit ef19035

6 files changed

Lines changed: 180 additions & 15 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 72 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package com.datadog.appsec.php
22

33
import groovy.transform.Canonical
44
import groovy.transform.ToString
5+
import groovy.transform.stc.ClosureParams
6+
import groovy.transform.stc.FromString
57
import java.net.http.HttpRequest
68
import java.net.http.HttpResponse
79
import java.util.Base64
@@ -13,12 +15,31 @@ import static java.net.http.HttpResponse.BodyHandlers.ofString
1315
* @link https://github.com/DataDog/instrumentation-telemetry-api-docs/blob/main/GeneratedDocumentation/ApiDocs/v2/producing-telemetry.md
1416
*/
1517
class TelemetryHelpers {
16-
static <T> List<T> filterMessages(List<Map> telemetryData, Class<T> type) {
17-
(telemetryData.findAll { it['request_type'] in type.names } +
18-
telemetryData.findAll { it['request_type'] == 'message-batch'}
19-
*.payload*.findAll { it['request_type'] in type.names }.flatten())
20-
.collect { type.newInstance([it['payload']] as Object[]) }
21-
}
18+
/**
19+
* Filters drained telemetry to messages of the given wrapper type, unwrapping any
20+
* {@code message-batch} envelopes.
21+
*
22+
* <p>The sidecar runs two telemetry workers per session: one for the host app and
23+
* one for {@code service_name == "datadog-ipc-helper"} (its own self-telemetry).
24+
* Tests almost always want only the former; pass {@code userAppOnly = false} to
25+
* include the sidecar's own telemetry too.
26+
*/
27+
static <T> List<T> filterMessages(List<Map> telemetryData, Class<T> type, boolean userAppOnly = true) {
28+
List<Map> payloads = []
29+
for (msg in telemetryData) {
30+
if (userAppOnly && msg.application?.service_name == 'datadog-ipc-helper') continue
31+
if (msg.request_type in type.names) {
32+
payloads << (msg.payload as Map)
33+
} else if (msg.request_type == 'message-batch') {
34+
for (inner in (msg.payload as List)) {
35+
if (inner.request_type in type.names) {
36+
payloads << (inner.payload as Map)
37+
}
38+
}
39+
}
40+
}
41+
payloads.collect { type.newInstance([it] as Object[]) }
42+
}
2243

2344
static class GenerateMetrics {
2445
static names = ['generate-metrics']
@@ -361,6 +382,19 @@ class TelemetryHelpers {
361382
}
362383
}
363384

385+
static class WithExtendedHeartbeat {
386+
static names = ['app-extended-heartbeat']
387+
List<ConfigurationEntry> configuration
388+
List<DependencyEntry> dependencies
389+
List<IntegrationEntry> integrations
390+
391+
WithExtendedHeartbeat(Map m) {
392+
configuration = m.configuration?.collect { new ConfigurationEntry(it) }
393+
dependencies = m.dependencies?.collect { new DependencyEntry(it) }
394+
integrations = m.integrations?.collect { new IntegrationEntry(it) }
395+
}
396+
}
397+
364398
static class ConfigurationEntry {
365399
String name
366400
String value
@@ -399,7 +433,10 @@ class TelemetryHelpers {
399433
}
400434
}
401435

402-
public static <T> List<T> waitForTelemetryData(AppSecContainer container, int timeoutSec, Closure<Boolean> cl, Class<T> cls, String path = '/hello.php') {
436+
static <T> List<T> waitForTelemetryData(AppSecContainer container, int timeoutSec,
437+
@ClosureParams(value = FromString, options = 'java.util.List<T>')
438+
Closure<Boolean> cl,
439+
Class<T> cls, String path = '/hello.php') {
403440
List<T> messages = []
404441
def deadline = System.currentTimeSeconds() + timeoutSec
405442
def lastHttpReq = System.currentTimeSeconds() - 6
@@ -422,23 +459,46 @@ class TelemetryHelpers {
422459
messages
423460
}
424461

425-
public static List<AppEndpoints> waitForAppEndpoints(AppSecContainer container, int timeoutSec, Closure<Boolean> cl, String path = '/') {
462+
static List<AppEndpoints> waitForAppEndpoints(AppSecContainer container, int timeoutSec,
463+
@ClosureParams(value = FromString,
464+
options = 'java.util.List<com.datadog.appsec.php.TelemetryHelpers.AppEndpoints>')
465+
Closure<Boolean> cl,
466+
String path = '/') {
426467
waitForTelemetryData(container, timeoutSec, cl, AppEndpoints, path)
427468
}
428469

429-
public static List<GenerateDistributions> waitForDistributions(AppSecContainer container, int timeoutSec, Closure<Boolean> cl) {
470+
static List<GenerateDistributions> waitForDistributions(AppSecContainer container, int timeoutSec,
471+
@ClosureParams(value = FromString,
472+
options = 'java.util.List<com.datadog.appsec.php.TelemetryHelpers.GenerateDistributions>')
473+
Closure<Boolean> cl) {
430474
waitForTelemetryData(container, timeoutSec, cl, GenerateDistributions)
431475
}
432476

433-
public static List<GenerateMetrics> waitForMetrics(AppSecContainer container, int timeoutSec, Closure<Boolean> cl) {
477+
static List<GenerateMetrics> waitForMetrics(AppSecContainer container, int timeoutSec,
478+
@ClosureParams(value = FromString,
479+
options = 'java.util.List<com.datadog.appsec.php.TelemetryHelpers.GenerateMetrics>')
480+
Closure<Boolean> cl) {
434481
waitForTelemetryData(container, timeoutSec, cl, GenerateMetrics)
435482
}
436483

437-
public static List<WithIntegrations> waitForIntegrations(AppSecContainer container, int timeoutSec, Closure<Boolean> cl) {
484+
static List<WithIntegrations> waitForIntegrations(AppSecContainer container, int timeoutSec,
485+
@ClosureParams(value = FromString,
486+
options = 'java.util.List<com.datadog.appsec.php.TelemetryHelpers.WithIntegrations>')
487+
Closure<Boolean> cl) {
438488
waitForTelemetryData(container, timeoutSec, cl, WithIntegrations)
439489
}
440490

441-
public static List<Logs> waitForLogs(AppSecContainer container, int timeoutSec, Closure<Boolean> cl) {
491+
static List<WithExtendedHeartbeat> waitForExtendedHeartbeat(AppSecContainer container, int timeoutSec,
492+
@ClosureParams(value = FromString,
493+
options = 'java.util.List<com.datadog.appsec.php.TelemetryHelpers.WithExtendedHeartbeat>')
494+
Closure<Boolean> cl) {
495+
waitForTelemetryData(container, timeoutSec, cl, WithExtendedHeartbeat)
496+
}
497+
498+
static List<Logs> waitForLogs(AppSecContainer container, int timeoutSec,
499+
@ClosureParams(value = FromString,
500+
options = 'java.util.List<com.datadog.appsec.php.TelemetryHelpers.Logs>')
501+
Closure<Boolean> cl) {
442502
waitForTelemetryData(container, timeoutSec, cl, Logs)
443503
}
444504
}

appsec/tests/integration/src/main/groovy/com/datadog/appsec/php/docker/AppSecContainer.groovy

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,6 @@ class AppSecContainer<SELF extends AppSecContainer<SELF>> extends GenericContain
9696
withEnv '_DD_DEBUG_SIDECAR_LOG_METHOD', 'file:///tmp/logs/sidecar.log'
9797
withEnv 'DD_SPAWN_WORKER_USE_EXEC', '1' // gdb fails following child with fdexec
9898
withEnv 'DD_TELEMETRY_HEARTBEAT_INTERVAL', '10'
99-
withEnv 'DD_TELEMETRY_EXTENDED_HEARTBEAT_INTERVAL', '10'
10099
// withEnv '_DD_SHARED_LIB_DEBUG', '1'
101100
if (System.getProperty('XDEBUG') == '1') {
102101
Testcontainers.exposeHostPorts(9003)
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
package com.datadog.appsec.php.integration
2+
3+
import com.datadog.appsec.php.TelemetryHelpers
4+
import com.datadog.appsec.php.docker.AppSecContainer
5+
import com.datadog.appsec.php.docker.FailOnUnmatchedTraces
6+
import groovy.util.logging.Slf4j
7+
import org.junit.jupiter.api.Test
8+
import org.junit.jupiter.api.condition.DisabledIf
9+
import org.testcontainers.junit.jupiter.Container
10+
import org.testcontainers.junit.jupiter.Testcontainers
11+
12+
import java.net.http.HttpResponse
13+
14+
import static com.datadog.appsec.php.integration.TestParams.getPhpVersion
15+
import static com.datadog.appsec.php.integration.TestParams.getVariant
16+
17+
/**
18+
* Standalone test class for the extended-heartbeat re-emission behaviour.
19+
*
20+
* The default extended-heartbeat interval is 24h. To exercise it within the test
21+
* timeout, this class starts its container with DD_TELEMETRY_EXTENDED_HEARTBEAT_INTERVAL=15
22+
* (kept separate from TelemetryTests so the persistent telemetry worker for that
23+
* class isn't already running with the long interval before this test runs).
24+
*
25+
* Per the spec (instrumentation-telemetry-api-docs/Source/ApiDocs/v2/openapi.yaml
26+
* `AppExtendedHeartbeat`), the payload must carry configuration + dependencies +
27+
* integrations so the agent can reconstruct application records on data loss.
28+
*/
29+
@Testcontainers
30+
@Slf4j
31+
@DisabledIf('isDisabled')
32+
class TelemetryExtendedHeartbeatTests {
33+
static boolean disabled = variant.contains('zts') || phpVersion != '8.4'
34+
35+
@Container
36+
@FailOnUnmatchedTraces
37+
public static final AppSecContainer CONTAINER =
38+
new AppSecContainer(
39+
workVolume: this.name,
40+
baseTag: 'apache2-fpm-php',
41+
phpVersion: phpVersion,
42+
phpVariant: variant,
43+
www: 'base',
44+
) {
45+
@Override
46+
void configure() {
47+
super.configure()
48+
// Short interval so the extended heartbeat fires within the test.
49+
withEnv 'DD_TELEMETRY_EXTENDED_HEARTBEAT_INTERVAL', '15'
50+
}
51+
}
52+
53+
@Test
54+
void 'extended heartbeat re-emits configuration, dependencies and integrations'() {
55+
// Trigger the integration loaders for phpredis (Redis class instantiation) and
56+
// exec (system() call). These cause the tracer to push AddIntegration to the
57+
// sidecar.
58+
CONTAINER.traceFromRequest('/custom_integrations.php?integrations[]=redis&integrations[]=exec') {
59+
HttpResponse<InputStream> resp -> assert resp.statusCode() == 200
60+
}
61+
62+
// Stage 1: verify the regular flush path actually emits phpredis (and exec)
63+
// in an app-started or app-integrations-change message. This proves we're
64+
// about to test the heartbeat re-emitting *previously-flushed* data.
65+
Set<String> flushed = [] as Set
66+
TelemetryHelpers.waitForIntegrations(CONTAINER, 30) { msgs ->
67+
msgs.each { (it.integrations ?: []).each { flushed << it.name } }
68+
'phpredis' in flushed && 'exec' in flushed
69+
}
70+
assert 'phpredis' in flushed : "phpredis not emitted via app-started/app-integrations-change; saw: ${flushed}"
71+
assert 'exec' in flushed : "exec not emitted via app-started/app-integrations-change; saw: ${flushed}"
72+
73+
// Stage 2: the extended heartbeat must re-emit the full triple
74+
// (configuration + dependencies + integrations).
75+
CONTAINER.drainTelemetry(0)
76+
def hbs = TelemetryHelpers.waitForExtendedHeartbeat(CONTAINER, 35) { !it.empty }
77+
assert !hbs.empty : 'No app-extended-heartbeat received within 35s'
78+
def hb = hbs.first()
79+
assert hb.configuration != null : 'heartbeat must include configuration field'
80+
assert hb.dependencies != null : 'heartbeat must include dependencies field'
81+
assert hb.integrations != null : 'heartbeat must include integrations field'
82+
assert 'phpredis' in hb.integrations*.name :
83+
"heartbeat should re-emit phpredis; got: ${hb.integrations*.name}"
84+
85+
// Stage 3: regression check for the leak that the libdd_telemetry fix addresses.
86+
// After the heartbeat, the worker should pop the re-queued items out of unflushed.
87+
// So when a NEW integration is loaded next, the following regular app-integrations-change
88+
// must contain only the new integration, NOT the previously-flushed-and-re-queued phpredis.
89+
CONTAINER.drainTelemetry(0)
90+
CONTAINER.traceFromRequest('/custom_integrations.php?integrations[]=fs') {
91+
HttpResponse<InputStream> resp -> assert resp.statusCode() == 200
92+
}
93+
Set<String> followup = [] as Set
94+
TelemetryHelpers.waitForIntegrations(CONTAINER, 30) { msgs ->
95+
msgs.each { (it.integrations ?: []).each { followup << it.name } }
96+
'filesystem' in followup
97+
}
98+
assert 'filesystem' in followup : 'No app-integrations-change with filesystem within 30s after heartbeat'
99+
assert !('phpredis' in followup) :
100+
"phpredis must not be re-emitted after the heartbeat; got: ${followup}"
101+
}
102+
}

appsec/tests/integration/src/test/www/base/public/custom_integrations.php

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ public function __construct()
1818
system('true');
1919
} elseif ($int === 'redis') {
2020
new Redis();
21+
} elseif ($int === 'fs') {
22+
// Triggers the filesystem integration via deferred-loaded fopen() hook.
23+
$fp = fopen('/dev/null', 'r');
24+
if ($fp) { fclose($fp); }
2125
} else {
2226
http_response_code(400);
2327
}

0 commit comments

Comments
 (0)