fix(tests): make payload-size integration suite deterministic, move backend-delivery checks to non-blocking suite - #1353
Conversation
lucaspimentel
changed the title
This comment has been minimized.
This comment has been minimized.
lucaspimentel
changed the title
bea29d5 to
9d3bab3
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 9d3bab3726
ℹ️ 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".
There was a problem hiding this comment.
🟡 Changes recommended
The blocking suite currently under-asserts the “invocations return 200” requirement and includes an unbounded timeout diagnostic log scan that can significantly prolong failing CI runs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes the payload-size integration testing more deterministic by separating extension-side regression checks (blocking) from backend trace-delivery checks (non-blocking), reducing intermittent CI failures caused by eventual consistency and downstream flakiness.
Changes:
- Refactors
payload-size(blocking) to assert only extension-side behavior via CloudWatch logs, adding polling to wait for log searchability. - Adds
payload-size-e2e(non-blocking viaallow_failure) to retain Datadog trace-delivery assertions with extended indexing polling. - Updates CDK app + GitLab test-suite matrix to deploy/run the new E2E suite in parallel with isolated resources.
File summaries
| File | Description |
|---|---|
| integration-tests/tests/utils/payload-size.ts | Extracts shared constants and sleep helper for both payload-size suites. |
| integration-tests/tests/utils/cloudwatch.ts | Adds countLogEvents to help diagnose “not searchable yet” vs “never logged.” |
| integration-tests/tests/payload-size.test.ts | Reworks blocking suite to use direct Lambda invocations + CloudWatch log polling and removes backend assertions. |
| integration-tests/tests/payload-size-e2e.test.ts | Introduces new E2E suite that polls Datadog until the large trace is fully indexed before asserting delivery. |
| integration-tests/bin/app.ts | Deploys a second PayloadSize stack instance for the E2E suite to avoid parallel-job resource sharing. |
| .gitlab/templates/pipeline.yaml.tpl | Marks payload-size-e2e as allow_failure: true via rules. |
| .gitlab/datasources/test-suites.yaml | Adds payload-size-e2e to the integration test suite matrix. |
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
8dbc6df to
d5b3170
Compare
The payload-size suite asserted on a single trace search right after the default 5-minute indexing wait. A ~10 MB span can take longer than that to become searchable, so the suite failed with 0 traces delivered even though the traces were in the backend. Poll for the first invocation's traces for up to 10 more minutes instead.
CloudWatch Logs is eventually consistent, so querying immediately after the invocations returned nothing and the size guards failed with undefined byte counts. Poll until the extension's debug lines become searchable, then run the 413 check.
- Wait for the full 400-span trace to be indexed instead of returning as soon as any trace appears, since a request_id match goes non-empty the moment the root span indexes, well before the payload spans do - Add a settle wait before checking for a 413 log line, since it is logged after the send and retries, later than the batch-size line the poll was keyed on - Cap the unbounded CloudWatch log scan used for diagnostics so it can't outlast the test it's diagnosing - Deduplicate the sleep() helper into the shared payload-size utils 🤖
d5b3170 to
3043f06
Compare
| # dropped downstream after a successful send); keep failures visible but | ||
| # non-blocking until the backend issue is fixed. | ||
| - if: '$TEST_SUITE == "payload-size-e2e"' | ||
| allow_failure: true |
There was a problem hiding this comment.
As the failure could be due to setup, deployment and flaky tests, allowing failure may unintentionally open merge gate if not due to flaky tests. Would it be better to split this job into 2 steps: one for setup/deployment/invocation with allow failure=false and one for only known flaky tests with allow failure=true. What do you think?
litianningdatadog
left a comment
litianningdatadog
left a comment
There was a problem hiding this comment.
Left some comment
Overview
The
payload-sizesuite has been flaking onmainsince it was introduced in mid-June (#1257): 14 of 37mainpipelines (38%) had at least one failedpayload-sizeattempt between Jun 17 and Sep 4, withretry: 2masking about half of those failures (7 of 37 pipelines ended red after retries). Investigation showed the extension is not at fault: it batches the ~10 MB payload in a single successful send with no 413s, but the backend intermittently drops or truncates the trace after intake. This PR splits the suite so the per-pipeline gate only asserts deterministic, extension-side behavior, while the backend-delivery checks remain running but non-blocking:payload-size(blocking)Invokes the Lambda directly (no Datadog trace queries, no indexing wait) and asserts, from extension logs only:
These assertions passed in every observed failing run, so they are stable and meaningful as a regression gate.
Polling for searchable extension logs
Splitting the suite also removed the incidental ~5-minute wait the old Datadog indexing poll provided. Because CloudWatch Logs is eventually consistent, the suite's
FilterLogEventsqueries ran seconds after the invocations and returned nothing, so the byte-size assertions sawundefinedand failed even on healthy runs. The blocking suite now polls every 10 seconds, for up to 5 minutes, until the extension'spayload size after enrichment/totalingdebug lines are searchable; the 413 query runs only after the size lines are searchable, keeping an empty result meaningful. If the poll times out, the failure output includes diagnostics (total log events in the window and the count of extensionTRACESlines) that distinguish "extension never logged" from "lines not yet searchable".payload-size-e2e(new,allow_failure: true)Deploys its own stack (second
PayloadSizestack instance, distinct Lambda and log group, since suite jobs run in parallel) and keeps the backend-delivery assertions:aws.lambdaroot span presentorder.processspans presentIt also owns the trace-indexing poller from the previous commit: the first invocation's ~10 MB trace can take longer than the default 5-minute indexing wait to become searchable, so it is polled for up to 10 additional minutes at 30-second intervals (fast path if already indexed; transient API errors retried; on timeout the last fetched data is returned so assertions fail with real output).
Failures in this suite stay visible in CI as non-blocking warnings, preserving evidence of the backend drop issue while keeping the per-PR gate green. Re-enabling it as blocking later is a one-line change (remove the
allow_failurerule).See #1257
Testing
npm run build(tsc) passes inintegration-tests/.payload-size.test.tsandpayload-size-e2e.test.ts.PayloadSizestack instances succeeds (full-app synth requires CI-built lambda jars, so verified per-stack)..gitlab/templates/pipeline.yaml.tplwith gomplate against the real datasources: matrix containspayload-size-e2e, and only that suite hasallow_failure: true.payload-sizerun timed out waiting for searchable log lines; the retry with the polling fix passed all 4 tests (fresh stack, same commit).