Skip to content

test(integration): add timeout golden coverage - #842

Open
joeyzhao2018 wants to merge 7 commits into
mainfrom
joey/timeout_span_integration_test
Open

joeyzhao2018 wants to merge 7 commits into
mainfrom
joey/timeout_span_integration_test

Conversation

@joeyzhao2018

@joeyzhao2018 joeyzhao2018 commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

Adds pre-migration timeout golden coverage to the local RIE integration suite:

  • manual-timeout: explicit tracer initialization followed by manual datadog(handler) wrapping.
  • cjs-timeout: the same handler through the DD_LAMBDA_HANDLER redirect.
  • A shared handler keeps a timeout.unfinished child span open beyond the real five-second RIE deadline.
  • Raw-log assertions require exactly one error-tagged Lambda span across all trace payloads, its unfinished child flushed before termination, the configured flush deadline, and runtime reset/kill evidence.
  • Nine checker regression tests, CI wiring, log/return goldens, and capture documentation.
  • Optional RIE_HTTP_TRANSPORT=container for local environments whose published host ports are unreachable.

No production library changes. The test-only Lambda runtime images are now pinned to multi-architecture digests; three existing expectations are refreshed for the reviewed Node 22/24 runtime update.

Motivation

Freeze the existing timeout contract before moving Lambda instrumentation into dd-trace. Checking all trace payloads prevents a matching span from hiding a duplicate invocation span in another trace.

Testing Guidelines

Validation on the merged pre-migration baseline 291fd14e (datadog-lambda-js 12.143.0):

  • Recaptured both timeout cases on Node 18/20/22/24/26, RIE v1.36, linux/arm64: dd-trace 5.126.0 on 18/20 and lockfile-resolved 6.15.0 on 22/24/26. All runtimes agree with the shared goldens.
  • A separate comparison-only rerun passed all 90 timeout invocations; hashes of all four timeout snapshot files remained unchanged.
  • Existing container-cjs and manual-throw-error snapshots passed unchanged on Node 22.
  • node --test integration_tests_local/check-timeout-logs.test.js: 9 passed.
  • Resolved workflow preserves main's runtime/architecture matrix and adds only the timeout-checker step.
  • Shell/JavaScript syntax, formatting, and whitespace checks passed.
  • Runtime-baseline follow-up: all 18 cases passed in comparison-only mode on Node 22 and Node 24 / linux-arm64 (324 invocations), using the exact image digests from CI run 36168116285. Refreshed snapshot hashes remained unchanged.
  • Verified that every pinned image manifest includes both linux/amd64 and linux/arm64. The image mapping rejects unsupported runtimes.

Additional Notes

The log differences are

  • the now-populated dd_trace version tag
  • removal of empty links: [] fields
  • ordinary RIE record ordering under the existing comparison.

No checker assertions or normalization rules were loosened; timeout return-value goldens are unchanged.

CI then exposed floating-runtime drift: local cached images were from August 28 while clean CI runners pulled newer Node 22/24 images. Reproduced both differences with CI's exact images and reviewed their runtime source. The proactive-init snapshots now retain the new structured runtime_worker_pool_initializing DEBUG record; the Node 24 thrown-error response now retains its requestId field with existing value normalization. No runtime log filtering or behavior assertions were weakened. The existing thrown-error log golden and all timeout goldens are unchanged by this follow-up. Test image digests are pinned in lambda_node_image_tag() so local and CI runs cannot silently diverge again.

The earlier cf751a76 capture used dd-trace 5.105.0 on all runtimes and was no longer the correct baseline after merging main. The README records current provenance and verification.

Local validation used RIE_HTTP_TRANSPORT=container to bypass broken Colima host-port forwarding. Native amd64 and real AWS timeout validation were not run locally. These are emulator goldens, not proof of deployed Lambda termination or layer packaging. CI run 36170985446 passed all ten native runtime/architecture jobs on 85fdb2ee: Node 18/20/22/24/26 on amd64 and arm64, including the no-snapshot-writes guard. Unrelated migration-planning edits remain outside this PR.

Types of Changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog
  • This PR passes the integration tests (local RIE suite: all ten native CI jobs passed; real AWS validation not run)

@datadog-prod-us1-5

This comment has been minimized.

@joeyzhao2018
joeyzhao2018 marked this pull request as ready for review September 25, 2026 18:56
@joeyzhao2018
joeyzhao2018 requested review from a team as code owners September 25, 2026 18:56

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 85fdb2ee23

ℹ️ 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".

assert.ok(index > startIndex && index < reports.get(requestId), "span exported before runtime timeout REPORT");
// The configured guard fires ~1.5s in, allowing 1s of scheduling overhead.
// Merely checking <5s would also accept a broken/default 100ms flush deadline.
assert.ok(span.duration > 0 && span.duration < 2.5e9, "span must honor the 3500ms flush deadline");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce a lower bound on the timeout span duration

The assertion accepts any positive duration below 2.5 seconds, so a regression that schedules the timeout callback immediately—such as an incorrect delay calculation or clamping to zero—would still pass while violating the intended roughly 1.5-second trigger. Since duration is removed from the golden during normalization, no other assertion catches this; add a reasonable lower bound as well as the existing upper bound.

Useful? React with 👍 / 👎.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant