Repository navigation
feat(telemetry): check the exact record shape against a golden record - #160
Conversation
ec67e69 to
cd4178e
Compare
cd4178e to
faa0491
Compare
faa0491 to
e1c42e9
Compare
This comment has been minimized.
This comment has been minimized.
e1c42e9 to
e0c0ea0
Compare
The validator checked only the keys a requirement listed. So it accepted an extra key at any level, an `operations` array next to `operationsByName`, truncation markers, a different key order, `count: 1.0` for `1`, a `result` on an operation summary, and timestamps with 0, 3, 6, or 9 fraction digits. Each of these is a difference between SDKs that the suite exists to detect. `entry.record` is now the complete expected record. At every mapping level the record must carry exactly the listed keys, in the listed order. A number matches only a number of the same JSON type. The identity fields come from six placeholders bound from the execution ARN: EXECUTION_ARN (bound by core), EXECUTION_NAME, FUNCTION_NAME, FUNCTION_QUALIFIER, REGION, ACCOUNT_ID. `forbid_fields` and `operations_by_name` are removed; the closed key set covers both, and `operationsByName` sits inside `record` at its position. telemetry-1 lists the 16 top-level keys and 9 summary keys in the order the JS CloudWatch Logs exporter writes them. Timestamps must have exactly three fraction digits and a Z suffix, the fixed-width form every SDK writes, so that text order equals time order in CloudWatch Logs Insights and DynamoDB. Closes #153
e0c0ea0 to
2795446
Compare
Codex AI reviewOne prior P2 finding remains confirmed (deferred); no additional findings. Tests and cloud validation were not run under the read-only review constraints. Suppressed repeats
Reviewed commit |
ParidelPooya
left a comment
There was a problem hiding this comment.
Approving. The validator now compares entry.record as a closed mapping at every level, checks key order, compares numbers by JSON type, and pins timestamps to YYYY-MM-DDTHH:MM:SS.sssZ.
I re-ran the mutation probe from the #144 review against this branch and against main. Every case this PR targets is now rejected:
- extra keys at the top level, in
input, and in an operation summary; operations[]next tooperationsByName;- truncation markers, and a summary
result; - any key-order change;
1.0in place of1;- timestamps with 0, 1, 6, or 9 fraction digits.
The golden record matches what the JS exporter writes: buildRecord, then withOperationsByName. Python main passes, in a local run of its real CloudWatchLogsExporter with a fake logs client. Java main fails on one point: OperationSummary.toWireMap() writes the summary keys in a different order from JS. That is a Java SDK bug, which this suite now catches as intended.
Non-blocking
-
The timestamp pattern accepts a trailing newline and non-ASCII digits. The validator compiles the pattern with default flags and calls
search. In Python,$also matches before a final\n, so2026-01-01T00:00:01.000Z\npasses. In a Python 3strpattern,\dmatches any Unicode decimal digit, so the same timestamp written in Arabic-Indic digits passes. The README says any other suffix fails, so the README claim is false as written. No SDK writes either form today. The fix is one line intelemetry-1.yaml: use[0-9]for\dand\Zfor$. Then add both values totest_telemetry_1_rejects_other_timestamp_formats. -
Fields are not checked against each other. The golden record checks each field alone. So these pass:
endTimebeforestartTime,emittedAtbeforestartTime,durationMsdifferent fromendTime - startTime,minDurationMsgreater thanmaxDurationMs, and a date with month 13. This can hide a real difference between SDKs:- JS computes
durationMsfrom the written millisecond timestamps. - Python computes it from the untruncated times.
- With an execution start of
00:00:00.000999, Python main wrote adurationMs1 ms smaller than the difference of its written timestamps.
I did not verify whether Lambda delivers start times with sub-millisecond precision. A small post-check in
validate_recordwould cover all five cases. - JS computes
-
A duplicated key in the message passes.
json.loadskeeps the last value at the position of the first key. So{"status":"FAILED", ..., "status":"SUCCEEDED"}validates asSUCCEEDED, with the correct key order. No SDK writes duplicate keys today. Rejecting them takes oneobject_pairs_hookinparse_record. That event would then count as malformed. -
The reserved
EXECUTION_ARNplaceholder is not protected. The Codex finding onextension.py:126is correct. Core overwritesEXECUTION_ARN, soVariables: {EXECUTION_ARN: ...}is accepted without an error. The other five names fail with the collision error, as intended. Deferring the fix to #165 is reasonable, because the cause is in core. -
The README should state the payload key-order limitation. The PR body says that Go's
encoding/jsonsorts map keys, so a multi-key payload from Go would fail the order check. The README says only thatinputandoutputkeys are checked in the listed order. A requirement author reads the README, not the PR body. So please copy the limitation into the README "What is asserted" section. Whether user payload key order is part of the contract is a team decision. Today the suite says it is. -
The PR body names a stack PR that does not exist. It says "Stack #162:
main→ this PR → #150". #162 does not exist in this repo. #150 is the PR based on this branch.
Not covered yet
These are outside this PR's scope. I list them so the remaining parity work stays visible.
- One scenario. Only a successful execution with one step is checked. None of these appear in a checked record:
- a failure, so the error shape
{name, message, cause?}is not checked; - retries, waits, and
on-changeRUNNING records; - truncation markers;
- repeated operation names;
- opted-in operation results;
- child contexts and
full-tree.
- a failure, so the error shape
- One exporter. Only
CloudWatchLogsExporteris checked.LambdaLogExporterrecords arrive inside a log envelope, and the parser drops them. - One language with a handler and a cloud run. Only JS has both. The reusable workflow is in #150. Python and Java
mainhave no telemetry conformance handler yet. Go was not checked.
Stack #162:
main→ this PR → #150. #144 has merged asf04647b. This PR adds one commit on top ofmain.Closes #153.
Problem
The validator checked only the keys a requirement listed. So it accepted an extra key at any level, an
operationsarray next tooperationsByName, truncation markers, a different key order,count: 1.0for1, aresulton an operation summary, and timestamps with 0, 3, 6, or 9 fraction digits. Each of these is a difference between SDKs that the suite exists to detect.Change
entry.recordis now the complete expected record.1.0does not match1.EXECUTION_ARN(bound by core),EXECUTION_NAME,FUNCTION_NAME,FUNCTION_QUALIFIER,REGION,ACCOUNT_ID. A requirement that binds one of these names underVariablesfails with a collision error.forbid_fieldsandoperations_by_nameare removed. The closed key set covers both.operationsByNamesits insiderecordat its position.telemetry-1.yamllists the 16 top-level keys and the 9 operation-summary keys in the order the JS CloudWatch Logs exporter writes them (buildRecordinaws-durable-execution-sdk-js-telemetry-exporter/src/index.ts). Timestamps must have exactly three fraction digits and aZsuffix. Every SDK writes this fixed-width form, so text order equals time order in CloudWatch Logs Insights and DynamoDB.Why a golden record in YAML, not JSON Schema
JSON Schema cannot express key order. It treats
1.0and1as the sameinteger. It admits an unlisted key unlessadditionalProperties: falseis written at every level. The existing YAML expectation language already has regex and placeholder rules. So the golden record reuses that language, and the validator gains the closed-mapping and key-order checks.Expected first result per SDK
operationsByName.incrementkey order.OperationSummary.toWireMap()writestype, subType, count, minDurationMs, maxDurationMs, totalDurationMs, failedCount, maxAttempt, status. JS writestype, count, failedCount, status, subType, minDurationMs, maxDurationMs, totalDurationMs, maxAttempt. That is a Java exporter difference for a follow-up issue in the Java repository.A failure on a real difference is the intended result. It is what #153 asks the suite to detect.
Known limitation
Key order is checked inside
inputandoutputtoo. Go'sencoding/jsonsorts map keys. So a Go record with a multi-key payload would fail the order check. telemetry-1 uses single-key payloads, so this has no effect today.Validation
hatch run test:all: 703 passed (telemetry package: 264, up from 199).hatch run types:check,hatch fmt --check,hatch run yaml:lint,hatch run dist:build+dist:check: pass.telemetry-1 PASSED. 1 record fetched, 0 malformed, 0 errors. The fetched record has the 16 keys in the golden order, an integerdurationMs, and three-digit timestamp fractions.operationsByName, addtruncated, moverecordTypelast,durationMs: 896.0, six-digitemittedAtfraction, wrongaccountId). Each produces exactly one error.