[codex] test(telemetry): deduplicate fork-cloned heartbeats#7304
Conversation
|
|
🎉 All green!🧪 All tests passed 🔗 Commit SHA: cc5c177 | Docs | Datadog PR Page | Give us feedback! |
a268fd3 to
cfb4e71
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a268fd3382
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cbeauchesne
left a comment
cbeauchesne
left a comment
There was a problem hiding this comment.
Do you mind rebasing, and unskip the test skipped by APMAPI-2167 ?
|
I already deleted the code from local 😬 |
|
@codex unskip the test skipped by APMAPI-2167 and commit back to this branch |
|
Summary
Testing
|
|
Thanks for the fix — dedup by (runtime_id, seq_id) is a solid, targeted fix for the fork-clone issue, and the regression test reproduces the exact scenario. A few things before merging:
|
35db15b to
94f46fd
Compare
|
@mabdinur would you like to take over the PR? I already deleted the PR fromy local system, and I assume it would be faster for you to apply the fixes and push them here 🙇 |
Dedup on (runtime_id, seq_id) alone treats a flattened message-batch entry as if it were a standalone message, but flattening keeps the outer batch's seq_id on every entry it contains. Two distinct heartbeats sent in the same batch would therefore collide and one would be silently dropped. Switch to unflattened telemetry data and key the dedup on (seq_id, batch_index) instead, and document why a seq_id collision is a reliable signal for a fork clone in this codebase (/spawn_child does a raw os.fork(), so parent and child share a runtime_id while each keeps its own seq_id counter copied from the same value at fork time). Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the batch-index-aware dedup fix, but drop the dedicated regression test for the message-batch collision scenario. Co-authored-by: Cursor <cursoragent@cursor.com>
…check Deduping by (seq_id, batch_index) only catches retries and fork clones that share a seq_id. Locally reproducing the flake (10+ runs of the full Test_Telemetry class against python/uwsgi-poc) showed a different, more common failure: a forked child from the session-id tests exits ~1 heartbeat interval after starting, but can emit an extra out-of-cadence heartbeat as part of its shutdown flush -- a real, distinct message with its own seq_id, so dedup can't touch it. With only 2-3 total samples, that single anomalous gap dominates the average and fails the assertion regardless of dedup. Add a min_lifespan floor (2x the configured heartbeat interval): runtimes that didn't live long enough to give a statistically meaningful cadence sample are excluded from measurement entirely, while long-lived runtimes with the same kind of anomaly are still measured and can still fail. This reproduced the flake reliably before the change and passed 10/10 local runs after it. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
Trim the heartbeat dedup/lifespan comments down to their essential reasoning, and add a selector rule so that changes to the standalone test_telemetry_heartbeat_utils.py helper (which has no test items of its own) correctly select the DEFAULT and TEST_THE_TEST scenarios in CI's diff-based scenario selection. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
3 participants
Summary
(runtime_id, seq_id)Why
This is a stacked follow-up to #7296.
The reported uWSGI fork race can emit a heartbeat under the long-lived parent's
runtime_idbefore the child regenerates its own ID. Filtering entire runtime buckets by lifespan therefore keeps the contaminated parent bucket and can leave the original flake in place.Repeated heartbeats with the same runtime and sequence ID represent one logical telemetry message, so counting that pair once removes the fork/retry clone without suppressing distinct fast heartbeats that the drift test should catch.
Validation
nix develop --command ./run.sh TEST_THE_TEST tests/test_the_test/test_telemetry_heartbeat.py— 2 passednix develop --command ./format.sh— all checks passed