test: expand local integration harness to old-suite coverage parity - #821
Merged
Conversation
PR 2a of the dd-trace migration roadmap: freeze goldens at the coverage level of the AWS-based suite, not just the two container variants. Cases (was 2, now 11): layer-cjs/layer-esm (new /opt layer fixture via prepare-layer.js, lockfile-pinned deps), manual-throw-error / manual-status-500 / manual-send-metrics / manual-process-input (AWS suite's manual-wrap handlers ported into the cjs fixture), cjs-http-requests (hermetic mock server on a per-run docker network), cjs-custom-extractor (DD_TRACE_EXTRACTOR, asserts _dd.parent_source), cjs-proactive-init (managed-instances eager init + 15s gap, raw-log marker assertions). Harness: run.sh case engine (CASE_PARAM; VARIANT_PARAM and SIMULATE_PROACTIVE_INIT kept as aliases), per-case return-value modes (default/case/per-event) with most-specific-first golden resolution; assert-then-collapse extended to the dd_lambda_layer node major; normalize.sh rules for ISO8601-T RIC timestamps, compact JSON trace headers, requestId forms, TimeoutOverflowWarning values, node pids, and the managed-instances supervisor pid. Per-runtime override goldens pin genuine divergences: Node 24+/26 error stack frames, Node 18 dns/net plugin spans, per-major TimeoutOverflowWarning emission in the proactive case. Verified: all 11 cases pass strict compare on nodejs 18/20/22/24/26.
|
Node appends "(Use `node --trace-warnings ...` to show where the warning was created)" to the first warning a process emits. Under the managed-instances path, which warning is first per process is racy, so the hint's distribution across TimeoutOverflowWarning records differs between runs — and between arm64 local and amd64 CI, which is what failed the node22/node24 legs on the PR. Drop the hint in both its forms (JSON-embedded `\n(...)` suffix and standalone line) in normalize.sh and re-capture the four proactive-init goldens that contain it. Verified: strict compare passes for cjs-proactive-init on all five runtimes.
![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: 6dd3d8e4bf
ℹ️ 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".
The pack step ran `yarn install --frozen-lockfile` directly. On a tree that pins dd-trace v6 (engines node >=22) with a host Node older than 22 — e.g. this repo's GitHub Actions runner image, which sets up Node 18.12 — that install fails with "The engine \"node\" is incompatible with this module", so every local integration test leg on a v6 PR fails before any container is built. Two changes, both no-ops on the current v5 tree: - Pack step: when scripts/install_deps.sh exists (v6 world), install through it with TARGET_NODE_MAJOR=$RUNTIME_PARAM (default 22) so the v5 fallback line is installed for older runtimes and --ignore-engines covers v6 on older hosts. The script restores package.json/yarn.lock on exit, so the worktree stays clean. Without it, fall back to the previous plain yarn install. - Container builds: pass a DD_TRACE_VERSION build-arg resolved through scripts/dd_trace_versions.sh (v5 line for node <22) so the fixture image installs a tracer its runtime can actually run. Current fixture Dockerfiles declare no such ARG, so docker only warns; v6-world fixtures consume it via npm pkg set. Verified: - v5 tree: RUNTIME_PARAM=22 CASE_PARAM=container-cjs passes end to end. - v6 tree (onzia/moreddtrace6 + this harness): node18 leg installs dd-trace 5.124.0 and node22 leg installs 6.12.0 on a Node 18 host (the original failure scenario), containers build and run; the only remaining diffs are the expected tracer-version snapshot drift (5.118 -> 5.124/6.x drop empty "links": [] and populate dd_trace meta), which the v6 PR owns regenerating, same as it already did for the AWS snapshots.
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
2 participants
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 this is
PR 2a of the dd-trace migration roadmap (Phase 0, "freeze the oracle"). PR 1 (#804) built
the local Docker/RIE harness with two container cases; the roadmap's PR 2 was scoped as
bookkeeping (tag + docs). Reviewing that scope against
migration_parity.mdsurfaced agap: the frozen goldens would have covered less than the AWS-based suite they are meant
to replace as the migration oracle. This PR widens the local harness so the frozen goldens
cover everything the old suite pinned — before any code moves.
It still moves no library code. It only adds fixtures, cases, and goldens, all captured
from the current implementation.
Coverage: old suite vs. this harness
The AWS suite (
integration_tests/serverless.yml) deploys 8 handler variants × 9 events ×5 runtimes. Every variant now has a docker-based counterpart:
container-cjs/container-esmcontainer-cjs/container-esmlayer-cjs/layer-esmlayer-cjs/layer-esmthrow-error-tracedmanual-throw-errordatadog()wrap, thrown errorstatus-code-500s-tracedmanual-status-500send-metricsmanual-send-metricssendDistributionMetricin/outside handlerprocess-input-tracedmanual-process-inputhttp-requests(-traced)cjs-http-requestshttp-requests(manual-wrap shape)manual-http-requestspatchHttpfallbackcjs-custom-extractorDD_TRACE_EXTRACTOR, asserts_dd.parent_source: eventcjs-proactive-initThe last two rows close parity rows that
migration_parity.mdlisted as Unprotected:the custom-extractor path (
_dd.parent_source) and proactive initialization are now pinnedby goldens + raw-log assertions instead of by hope. Still deliberately out of scope here
(left to unit/L3 as planned):
DD_TRACE_DISABLED_INSTRUMENTATIONS=lambda, the ESMdouble-registration guard, and direct-API/KMS paths.
What it took
Layer-mode fixture (
integration_tests/container/layer/). The real layer ships a zipmounted at
/opt; locally we reproduce the load path, not the packaging:prepare-layer.jsassembleslayer_pkg/from the repo build in the release Dockerfile'slayout (
dist/+handler.mjs+ the ESMmodule_importeroverlay) and pins dependencyversions from the lockfile-resolved
node_modulesset, so the fixture drifts with the repoinstead of away from it. The dependency manifest mirrors the release build's full
dependency closure — production dependencies plus every package
scripts/move_ddtrace_dependency.jsmoves (dd-trace, @datadog/native-appsec,@datadog/pprof, @opentelemetry/api, @opentelemetry/api-logs) — so the local layer image has
the same dependency closure as the release layer. The image installs it at
/opt/nodejs/node_modules/datadog-lambda-jsand sets the handler to/opt/nodejs/node_modules/datadog-lambda-js/handler.handler, exactly like a layer user.Build order matters:
npm installmust run before the fixture'sCOPY layer_pkg, or npmprunes it.
Manual-wrap handlers, ported as-is from
integration_tests/*.jsinto the cjs fixture,with one finding worth writing down: manual-wrap cases without a userland
dd-trace.init()produce no traces (No Tracer available, cannot start span) — and theold AWS snapshots show the same (
throw-error-traced/http-requestshavetraces=0intheir assertions). So the absence of trace JSON in the new goldens is faithful parity, not
a local-harness gap; the cases still pin error metrics, return bodies, and log output.
cjs-http-requestsruns in redirect mode, not manual wrap, for the reason above:traces are what make injected downstream headers observable. The AWS suite hits real
endpoints; locally a mock server on a per-run docker network (the Lambda base image with
--entrypoint node) echoes received headers, so the golden pinsx-datadog-*,traceparent/tracestate, and log-injection fields deterministically. The fixture readsMOCK_HTTP_URLSso the same handler code works in both suites.manual-http-requestscovers the other http-requests shape (review feedback): the AWSsuite's function is manually wrapped with no userland init, which routes outbound
injection through the library's own
patchHttpfallback (src/trace/patch-http.ts),gated on
autoPatchHTTP && !tracerInitialized. Running this shape through the redirectentry would skip that fallback entirely — a migration could break manual-wrapper HTTP
injection with every local test green. The golden pins what is observable under RIE
(patchHttp wraps and logs every request; the mock echo pins the exact header set); the
context-dependent header values remain pinned by the
patch-httpunit tests and the AWSsuite, because
_X_AMZN_TRACE_IDcannot be emulated under RIE (documented in the harnessREADME's emulation-gaps section).
cjs-proactive-initpromotes the PR 1 diagnostic (SIMULATE_PROACTIVE_INIT) into agated case: managed-instances eager init (
AWS_LAMBDA_MAX_CONCURRENCY=1) + a 15 sinit→invoke gap, with the three markers (
"proactive_initialization":1,proactive_initialization:true,cold_start:false) grep-asserted on the raw logs sonormalization can never hide them. The old knobs remain as aliases
(
VARIANT_PARAM=cjs|esm,SIMULATE_PROACTIVE_INIT=true).Harness mechanics.
run.shgains a case engine (CASE_PARAM, bash-3.2-safe) andper-case return-value modes (
default/case/per-event) with most-specific-firstgolden resolution. Assert-then-collapse is extended: the
dd_lambda_layer:datadog-nodevNNmajor is now verified against the runtime under test before collapsing to
nodevXX.XX.X, mirroring theruntime:nodejsNN.xcheck.normalize.shadds scoped rulesfor ISO8601-T RIC timestamps, compact-JSON trace headers, both
requestIdspellings,TimeoutOverflowWarningvalues,(node:NN)pids, and the managed-instances supervisorpid — each rule verified as a no-op against every previously committed golden.
Racy warning hint normalized. Node appends "(Use
node --trace-warnings ...)" to thefirst warning a process emits; under managed instances, which warning is first per
process is racy, so the hint's distribution across
TimeoutOverflowWarningrecords differedbetween runs — and between arm64 local and amd64 CI, which is what failed the node22/node24
legs mid-review. Normalization now strips the hint line wherever it appears.
dd-trace v6 trees stay runnable (forward compatibility)
The harness lives on
main, sopull_requestruns of other branches execute thisrun.shagainst their tree. On a tree that pins dd-trace v6 (engines.node >=22) with aCI host on Node 18, the old pack step (
yarn install --frozen-lockfile) fails before anycontainer builds. Two changes, both no-ops on this v5 tree:
scripts/install_deps.shwithTARGET_NODE_MAJOR=$RUNTIME_PARAMwhen that script exists (v6 world: v5 fallback linefor older runtimes,
--ignore-enginesfor v6 on older hosts), falling back to plainyarn installotherwise;DD_TRACE_VERSIONbuild-arg resolved throughscripts/dd_trace_versions.sh; this repo's fixture Dockerfiles declare no such ARG(docker only warns), while v6-world fixtures consume it to pin a tracer their runtime can
run.
Verified against the actual v6 branch (onzia/moreddtrace6 + this harness): the node18 leg
installs dd-trace 5.124.0 and the node22 leg installs 6.12.0 on a Node 18 host — the exact
scenario that failed — containers build and run, and the only remaining diffs are the
expected tracer-version snapshot drift that the v6 PR owns regenerating.
Per-runtime divergences (pinned, not absorbed)
Real behavioral differences across Node majors get override goldens, per the harness's
"divergence is a new file" rule:
manual-throw-error_node{24,26}.{log,json}— Node 24+ add their own frames(
BufferedInvokeProcessor, asyncindex.mjsframes) to error stack traces; the node26preview RIC also embeds a per-invocation
requestIdin the error body (normalized).cjs-http-requests_node18.log— dd-trace'sdns/netplugins fire on Node 18 only,adding
_dd.integrationspan meta.manual-http-requests_node18.log— same Node-18-onlydns/netplugin behavior on thefallback path.
cjs-proactive-init_node{18,20,24,26}.log—TimeoutOverflowWarningemission (countand JSON-record wrapping) varies by major under managed instances. The case's actual
assertions are the raw-log markers, identical on every runtime.
Verification
All 12 cases pass strict compare mode on nodejs 18/20/22/24/26 (60 legs), run twice where
goldens were re-captured. CI matrix is unchanged — the workflow comment now reflects that
each leg runs all cases.
Sequencing
PR 2b extends golden coverage beyond the old suite (callback-style handlers, fetch/undici
injection,
DD_TRACE_ENABLED=false,DD_CAPTURE_LAMBDA_PAYLOAD) and lands after this one;all goldens in both PRs are captured from pre-migration code.
Trying it locally
./integration_tests_local/run.sh # all 5 runtimes, all 12 cases RUNTIME_PARAM=18 CASE_PARAM=layer-esm ./integration_tests_local/run.sh UPDATE_SNAPSHOTS=true RUNTIME_PARAM=22 CASE_PARAM=cjs-http-requests ./integration_tests_local/run.shRequires Docker. First run pulls the base images (~1 GB each) and the RIE binary.