Skip to content

test(telemetry): replace hand-written telemetry expectations with record/replay goldens - #1640

Merged
wolo-lab merged 2 commits into
google:mainfrom
RKest:test/telemetry-functional-scenarios
Oct 1, 2026
Merged

wolo-lab merged 2 commits into
google:mainfrom
RKest:test/telemetry-functional-scenarios

Conversation

@RKest

@RKest RKest commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Please ensure you have read the contribution guide before creating a pull request.

Linked issue

Problem:
The functional telemetry tests compared each run against hand-written Go literals in internal/telemetry/telemetrytestcase. Only one content-capture mode was covered, and every telemetry change meant hand-editing expected span trees. The expectations could also not be compared with adk-python's recordings.

Solution:
Record/replay goldens, following adk-python's test_functional.py:

  • Golden: internal/telemetry/functionaltest/testdata/<test_id>.json, where the id is <scenario>/[<variant>-]<capture>-<schema>, holds the span tree, with each log record nested under the span it was emitted in. The digest shape (root_span / name / attributes / status / children / logs, plus the PRESENT sentinel) follows adk-python's _digests.py, with one difference: a JSON-string attribute is wrapped as {"JSON_STRING": …}, so it stays distinct from a structured one.
  • Replay: plain cmp.Diff between the recording and the golden. Recording fails on a log record emitted outside any span, and a golden that no case records fails the suite.
  • Re-record: go generate ./internal/telemetry/functionaltest.

Scenarios are typed, and data is kept apart from setup:

internal/telemetry/functionaltest/scenarios/
  testcasedata/          data only
    testcasedata.go      Scenario interface, Capture, SchemaVersion, Case, Matrix
    toolcall/            llmagent + function tool; Failure: inference-error, tool-error
    streaming/           two streamed chunks
    delegation/          agent-as-tool
    dynamicworkflow/     static node, dynamic node, agent nodes, cached node;
                         Failure: static-node-error, dynamic-node-error,
                         first-agent-error, second-agent-error
  testcaseimpl/          turns a Scenario into agents, models and tools
    common.go            Run (real Runner), Cases, the Scenario → builder switch
    toolcall.go …        one builder per scenario

Each scenario package has its own strongly typed Failure (or none) and a Matrix() covering every variant under:

  • every capture mode: no_content, span_only, event_only, span_and_event;
  • both ADK_TELEMETRY_SCHEMA_VERSION_OPT_IN values: otel_semconv_1_36 and otel_semconv_1_44.

That comes to 80 goldens. main does not read the schema variable yet, so each otel_semconv_1_36 / otel_semconv_1_44 pair is identical here. #1635, which starts reading it, then changes only otel_semconv_1_44 goldens, which shows exactly what telemetry changes and that the legacy format does not.

Behavior change

Nothing. This PR changes tests only.

Testing Plan

Unit Tests:

  • All unit tests pass locally: the AGENTS.md module loop (go mod tidy -diff, go build, go test -race -shuffle=on, golangci-lint run v2.3.1) is clean in both modules.

Coverage kept: each deleted expectation has a golden that pins at least as much:

  • AgentWithToolCase → toolcall/no_content-otel_semconv_1_36.json
  • AgentWithToolCaptureContentCase → toolcall/span_and_event-otel_semconv_1_36.json
  • the five Workflow*Cases → dynamicworkflow/*no_content-otel_semconv_1_36.json

The goldens pin the legacy tool arguments and responses by value; the old expectations had PRESENT there.

Determinism: 5 re-recordings under -shuffle=on produced byte-identical goldens.

With your source change reverted and your tests kept, which test fails?
Not applicable: there is no source change. The goldens pin span names, attributes, status codes, the tree shape, and log event names, bodies and attributes. Like adk-python's digest, they do not pin span events or status descriptions.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code, plus one fresh-context review pass; its findings are addressed.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end. (Not applicable: tests only.)
  • Any dependent changes have been merged and published in downstream modules.

@RKest
RKest force-pushed the test/telemetry-functional-scenarios branch from 41746ab to fd20f6f Compare September 24, 2026 15:04
@RKest
RKest marked this pull request as ready for review September 25, 2026 11:40
@RKest
RKest force-pushed the test/telemetry-functional-scenarios branch from fd20f6f to f802979 Compare September 25, 2026 11:48

@knapg knapg 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.

overall, lgtm!

@wolo-lab wolo-lab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One thing to fix before merge, and it is about the tests rather than the code: the new check that fails a recording when a log record is emitted outside any span has no test that exercises it.

Before this change the digest helper dropped such records silently by design (span_digest.go at the merge base, lines 102-109). Now BuildDigest fails instead, and the description lists that as one of the suite's guarantees. Replacing that t.Fatalf with continue leaves the whole suite green, and internal/telemetry/telemetrytest has no test file. So a later edit could quietly bring back the old drop. The check does work: a throwaway test that emits a record with no matching span hits the fatal. One thing to plan for when adding the test is that BuildDigest takes a concrete *testing.T, so asserting that it fails needs an error return or a testing.TB seam.

…ord/replay goldens

The functional telemetry tests compared each run against Go literals in
telemetrytestcase, so every telemetry change meant hand-editing expected
trees, and only one capture mode was covered. They now record the span
tree of a run, with its log records nested under the span they were
emitted in, as a golden, and replay it exactly. The digest follows
adk-python's _digests.py, except that a JSON-string attribute is wrapped
as {"JSON_STRING": ...}, so it stays distinct from a structured one.

Scenarios are typed: each has a package under scenarios/testcasedata
holding only its data (its own failure modes, and a Matrix of every
variant under every content capture mode and schema version), and
scenarios/testcaseimpl turns one into the agents, models and tools it
runs.

Every case is recorded under ADK_TELEMETRY_SCHEMA_VERSION_OPT_IN set to
otel_semconv_1_36 and to otel_semconv_1_44. ADK does not read the variable
yet, so each pair is identical; the change that starts reading it shows
exactly which telemetry changes, and that the legacy format does not.

Re-record with: go generate ./internal/telemetry/functionaltest
@RKest

RKest commented Sep 30, 2026

Copy link
Copy Markdown
Contributor Author

One thing to fix before merge, and it is about the tests rather than the code: the new check that fails a recording when a log record is emitted outside any span has no test that exercises it.

Before this change the digest helper dropped such records silently by design (span_digest.go at the merge base, lines 102-109). Now BuildDigest fails instead, and the description lists that as one of the suite's guarantees. Replacing that t.Fatalf with continue leaves the whole suite green, and internal/telemetry/telemetrytest has no test file. So a later edit could quietly bring back the old drop. The check does work: a throwaway test that emits a record with no matching span hits the fatal. One thing to plan for when adding the test is that BuildDigest takes a concrete *testing.T, so asserting that it fails needs an error return or a testing.TB seam.

Reverted the old behavior thanks

@RKest
RKest requested a review from wolo-lab September 30, 2026 18:07
@RKest
RKest force-pushed the test/telemetry-functional-scenarios branch from f802979 to df9d093 Compare September 30, 2026 18:08

@wolo-lab wolo-lab left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks!

@wolo-lab
wolo-lab merged commit aaabb3d into google:main Oct 1, 2026
14 checks passed
tongxury pushed a commit to tongxury/adk-go that referenced this pull request Oct 3, 2026
…ord/replay goldens (google#1640)

The functional telemetry tests compared each run against Go literals in
telemetrytestcase, so every telemetry change meant hand-editing expected
trees, and only one capture mode was covered. They now record the span
tree of a run, with its log records nested under the span they were
emitted in, as a golden, and replay it exactly. The digest follows
adk-python's _digests.py, except that a JSON-string attribute is wrapped
as {"JSON_STRING": ...}, so it stays distinct from a structured one.

Scenarios are typed: each has a package under scenarios/testcasedata
holding only its data (its own failure modes, and a Matrix of every
variant under every content capture mode and schema version), and
scenarios/testcaseimpl turns one into the agents, models and tools it
runs.

Every case is recorded under ADK_TELEMETRY_SCHEMA_VERSION_OPT_IN set to
otel_semconv_1_36 and to otel_semconv_1_44. ADK does not read the variable
yet, so each pair is identical; the change that starts reading it shows
exactly which telemetry changes, and that the legacy format does not.

Re-record with: go generate ./internal/telemetry/functionaltest

Co-authored-by: wolo <wolo@google.com>
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.

3 participants