fix(ci): patch run-tests.php getTimer() to return float instead of comma-formatted string#3856
Open
Leiyks wants to merge 10 commits into
Open
fix(ci): patch run-tests.php getTimer() to return float instead of comma-formatted string#3856Leiyks wants to merge 10 commits into
Leiyks wants to merge 10 commits into
Conversation
🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: fdf1556 | Docs | Datadog PR Page | Give us feedback! |
Leiyks
force-pushed
the
leiyks/fix-ci-run-tests-number-format
branch
3 times, most recently
from
a52010c to
fe39d11
Compare
Benchmarks [ tracer ]Benchmark execution time: 2026-05-15 13:24:37 Comparing candidate commit fdf1556 in PR branch Found 0 performance improvements and 13 performance regressions! Performance is the same for 180 metrics, 1 unstable metrics. scenario:ContextPropagationBench/benchInject64Bit
scenario:PDOBench/benchPDOOverhead
scenario:PDOBench/benchPDOOverheadWithDBM
scenario:PHPRedisBench/benchRedisOverhead
scenario:SamplingRuleMatchingBench/benchGlobMatching1
scenario:SamplingRuleMatchingBench/benchGlobMatching2
scenario:SamplingRuleMatchingBench/benchGlobMatching3
scenario:SamplingRuleMatchingBench/benchGlobMatching4
scenario:SamplingRuleMatchingBench/benchRegexMatching1
scenario:SamplingRuleMatchingBench/benchRegexMatching2
scenario:SamplingRuleMatchingBench/benchRegexMatching3
scenario:SamplingRuleMatchingBench/benchRegexMatching4
scenario:SpanBench/benchOpenTelemetryInteroperability
|
Leiyks
force-pushed
the
leiyks/fix-ci-run-tests-number-format
branch
4 times, most recently
from
e63b1f6 to
cce2c18
Compare
…= 7.2) Lists tests slower than 10s with their durations at the end of the run. The --show-slow flag only exists in PHP 7.2+, so it's gated by a runtime PHP_VERSION_ID check to avoid breaking PHP 7.0/7.1 jobs. Used to verify the hypothesis that recent CI failures are caused by tests exceeding 1000s under valgrind (triggering the number_format E_WARNING in run-tests.php's record() method).
…lgrind tests PHP's run-tests.php getTimer() returns number_format($elapsed, 4) which produces a comma-formatted string (e.g. "1,500.0000") when a test takes >=1000 seconds. PHP 8.0+ raises E_WARNING when that string is used in += arithmetic inside record(), causing the test runner to abort with exit code 2. Patch getTimer() at build time so it returns a float (via round()) instead.
The CI .base_test before_script runs wait-for-service-ready.sh, but WAIT_FOR only listed test-agent — request-replayer was never gated on, even though it is in the agent_httpbin_service() service block used by test_extension_ci, ASAN test_c, and ASAN test_c with multiple observers. Add a request-replayer case to detect_service_type (probing /replay, a read-only endpoint that returns valid JSON when php -S is up), and add request-replayer:80 to WAIT_FOR for the three affected job blocks. PHP Language Tests is unchanged: it only declares test-agent in services.
curl -sf fails on 4xx, which would falsely report not-ready if /replay returns 4xx when no dump exists yet (the natural startup state). Drop -f so any HTTP response proves php -S is up and executing index.php; connection failure remains the only signal of an unhealthy service.
Filters the SLOW TEST SUMMARY to tests that meaningfully approach the per-test 300s timeout, trimming noise from the long tail.
On PHP 8.4 the test sandbox integration registers early enough that the worker bundles it into the app-started payload's `integrations` field (see libdd-telemetry build_app_started), so no separate app-integrations-change event is emitted and the existing assertion loop times out. Accept both delivery paths: the dedicated app-integrations-change event, and the integrations array embedded in app-started. The expected output shape is identical because Rust Integration struct field order matches.
…ix flaky PHP 8.0 valgrind The test guarded a sleep(20) with `if (ini_get(...) != 0.5)`, but the wrapped sleep() blocks SIGVTALRM during the call. Under valgrind on PHP 8.0, the heavier signal traffic can cut the sleep short before the sidecar fetches the config from the request-replayer and propagates it to INI globals, leaving every dynamic value at its default. Replace the single sleep() with a 100ms polling loop bounded at 20s total. Each usleep() iteration goes through the same SIGVTALRM-blocking wrapper, which calls ddtrace_check_for_new_config_now() on unblock under valgrind, giving the test repeated, well-defined opportunities to apply the config while still finishing early on the happy path.
The previous warmup was a no-op:
$start = microtime(true);
...
usleep(floor(microtime(true) - $start) * 100000);
floor() of a sub-second float yields 0, so usleep(0) did nothing. Also,
the sleep happened *after* the span had already been sampled with an
empty env, so even a real delay wouldn't have helped.
Replace it with dd_trace_internal_fn('await_agent_info') called before
opening the span — same pattern that client_side_stats*.phpt already
uses to gate on the sidecar having read /info from the agent.
The request-replayer runs php -S, which is single-threaded by default.
With DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS=0.01 in many live-debugger
tests and parallel test workers under valgrind, the sidecar's RC polling
saturates the replayer's accept loop. Inbound connections from the test
process (e.g. file_get_contents('/replay')) hit the kernel listen backlog
and time out with 'Operation now in progress' (EINPROGRESS), poisoning
the EXPECTF output of any test that retries.
PHP 7.4+ recognizes PHP_CLI_SERVER_WORKERS to fork N worker processes,
restoring real concurrency for the built-in server.
Leiyks
force-pushed
the
leiyks/fix-ci-run-tests-number-format
branch
from
cb473bd to
67144f9
Compare
The previous PHP_CLI_SERVER_WORKERS=4 change broke InstrumentationTest::testInstrumentation across many test_web_custom jobs: the request-replayer image writes shared state to a single dump.json file with no inter-process coordination, so multiple php -S workers race and overwrite telemetry events (e.g. drop the 'logs_created' metric the test asserts on). Revert the workers var and instead reduce the per-test RC poll rate 10x (0.01s -> 0.1s) in the 12 affected tests. This cuts the sidecar's load on the single-process replayer enough to relieve the listen backlog under valgrind without breaking shared-state assumptions.
Leiyks
force-pushed
the
leiyks/fix-ci-run-tests-number-format
branch
from
0e715c5 to
fdf1556
Compare
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
1 participant
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.
What
sedpatch to$(BUILD_DIR)/run-tests.phpat build time, replacingnumber_format(withround(insidegetTimer()Why
PHP's
run-tests.phpgetTimer()returnsnumber_format($elapsed, 4), which produces a comma-formatted string (e.g."1,500.0000") when a test takes ≥1000 seconds. PHP 8.0+ raisesE_WARNING: A non-numeric value encounteredwhen that string is used in+=arithmetic insiderecord(). The main test runner process treats this as a fatal worker error and exits with code 2.This started surfacing ~1 week ago because commit fdbad29 moved
DD_EXPERIMENTAL_PROPAGATE_PROCESS_TAGS_ENABLED=0fromALL_TEST_ENV_OVERRIDE(covers all test targets, including valgrind) intoENV_OVERRIDE(covers only the non-valgrind run). The valgrind run now executes with process tags enabled, making live-debugger and sidecar-related tests 5–20× slower under valgrind's instrumentation, pushing some past the 1000s threshold.How
We already patch
run-tests.phpat build time for an unrelateddl()issue. This adds a second patch in the same$(BUILD_DIR)/run-tests.phpMakefile target:round($elapsed, 4)returns afloat(e.g.1999.5678) instead of a comma-formatted string, which is safe for+=arithmetic. The JUnit XMLtime='...'attribute also benefits —1999.5678is more spec-compliant than1,999.5678.Verified the sed pattern matches both PHP 8.4.19 (line 3441) and PHP 8.5.0 (line 3433).
Testing
test_extension_ci: [8.4]andtest_extension_ci: [8.5]valgrind runs no longer abort withE_WARNING: A non-numeric value encountered in run-tests.php