Skip to content

Test: Pin the ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS tracing gate with hermetic unit tests - #308

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/tracing-capture-env-gate-tests
Open

Test: Pin the ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS tracing gate with hermetic unit tests#308
AmaadMartin wants to merge 2 commits into
mainfrom
fix/tracing-capture-env-gate-tests

Conversation

@AmaadMartin

Copy link
Copy Markdown
Owner

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

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Problem: core/src/telemetry/tracing.ts has a module-private privacy kill switch, shouldAddRequestResponseToSpans(), that decides whether ADK writes potentially PII-bearing request/response payloads into OpenTelemetry spans. It is read from ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS. Two defects:

  1. Zero coverage. Grepping the repository for ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS returned exactly one hit — core/src/telemetry/tracing.ts:408. All six gated call sites (tracing.ts:122, :148, :189, :228, :247, :309) were only ever executed on the default capture-ON path.
  2. The suite was not hermetic. core/test/telemetry/tracing_test.ts never stubbed the variable, so three of its assertions silently depended on the ambient environment of the machine running them. Note vi.restoreAllMocks() does not unstub env vars and unstubEnvs is not enabled in vitest.config.ts, so this has to be explicit.

Reproduction, before this change:

$ npx vitest run --project unit:core                                          # 2205 passed
$ ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=false npx vitest run --project unit:core
 FAIL core/test/telemetry/tracing_test.ts > ... > traceToolCall > should set correct attributes for tool call
 FAIL core/test/telemetry/tracing_test.ts > ... > traceToolCall > should handle tool call without function response
 FAIL core/test/telemetry/tracing_test.ts > ... > traceCallLlm > should set correct attributes for LLM call
 Test Files  1 failed | 159 passed (160)
      Tests  3 failed | 2205 passed (2208)

After this change all three env states pass identically (2241 passed); see Testing Plan.

Solution: A test-only characterization change touching exactly one file, core/test/telemetry/tracing_test.ts. core/src/telemetry/tracing.ts is untouchedgit diff main --stat -- core/src prints nothing, and git diff main --stat is a single test file (+227/-1).

  • Pin the variable to its default in beforeEach (vi.stubEnv(CAPTURE_ENV_VAR, undefined) deletes it in Vitest 3) and vi.unstubAllEnvs() in afterEach, making the suite immune to ambient environment. Existing test bodies are unchanged.
  • Add a describe('ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS gate', ...) matrix over the eight values the implementation distinguishes, each exercised against all four public entrypoints (traceCallLlm, traceToolCall, traceMergedToolCalls, traceSendData).
  • Add one test asserting the gate redacts payloads without suppressing ungated telemetry.

The matrix, and why each row exists:

envValue capture why
undefined (deleted) on undefined || 'true''true'
'' on '' is falsy, so || 'true' applies
'true' on first disjunct
'1' on second disjunct
'false' off neither disjunct
'0' off neither disjunct
'TRUE' off the comparison is case-sensitive
'not-a-boolean' off any unrecognized value disables capture

These tests pin what the code does today, not what its JSDoc claims. The docstring on shouldAddRequestResponseToSpans says capture is disabled "only when ... explicitly set to 'false' or '0'", but the implementation is envValue === 'true' || envValue === '1', so 'TRUE' and 'not-a-boolean' also disable it. The 'TRUE' and 'not-a-boolean' rows deliberately encode the implementation, and Mutation B below shows they are the only tests that would notice if someone "fixed" the code to match the docstring. Reconciling the docstring with the implementation is intentionally out of scope here and is tracked separately.

Design notes:

  • The private helper stays private. Every assertion is driven through the four exported trace functions; nothing exports shouldAddRequestResponseToSpans "for the test", and no obj['privateField'] access is used.
  • collectSpanAttributes flattens the span recording so an assertion need not know whether an attribute arrived via setAttributes (bulk) or setAttribute (single). No gated key arrives via both forms. It is typed with Vitest's Mock<...> — no any.
  • traceToolCall/traceMergedToolCalls hardcode gcp.vertex.agent.llm_request/llm_response to '{}' and do not gate them (tracing.ts:120-121, 183-184). Asserting '{}' on those in the capture-disabled case would pass either way, so only genuinely gated keys are asserted.
  • traceSendData currently has no caller in adk-js (its live flow is still a stub) but is an exported entrypoint that reads the gate, so it is covered. It is not wired into any flow here.
  • No new suppressions. git diff main -U0 | grep -E '@ts-expect-error|@ts-ignore|eslint-disable|as any|: any\b|as unknown as|v8 ignore' returns nothing. The single pre-existing // eslint-disable-next-line @typescript-eslint/no-explicit-any on let mockSpan: any is untouched and was not widened.

One disclosure: npm run ts:check is not a CI gate today and reports 314 pre-existing errors repo-wide (8 of them already in this file). My new call sites add 6 more of the identical pre-existing class — fixtures typed via @google/adk (which plain tsc resolves to core/dist/types) passed into functions imported from core/src. This is the ts:check dist/src resolution problem, not a defect in these tests; the only local "fix" would be a cast, which would be a type-checker suppression. I deliberately did not add one, and followed the pattern already established at the 8 existing sites in the same file.

Collision check (required before starting): gh pr list --repo AmaadMartin/adk-js --state open --limit 100 plus gh pr diff --name-only across all 100 open PRs found no PR touching core/src/telemetry/tracing.ts or core/test/telemetry/tracing_test.ts. Two PRs are thematically adjacent but disjoint in files: #302 (tests/hermetic_env*.ts, vitest.config.ts) and #281 (core/test/telemetry/setup_test.ts, vitest.config.ts). This change does not touch vitest.config.ts and uses explicit vi.unstubAllEnvs(), so it is compatible with either landing first. Branched from main, not stacked.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

# Targeted
$ npx vitest run --project unit:core core/test/telemetry/tracing_test.ts
  Test Files  1 passed (1)
       Tests  39 passed (39)          # was 6

# Full project
$ npx vitest run --project unit:core
  Test Files  160 passed (160)
       Tests  2241 passed (2241)

# Hermeticity proof -- all pass identically (before: 3 failed)
$ ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=false   npx vitest run --project unit:core   # 2241 passed
$ ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=garbage npx vitest run --project unit:core   # 2241 passed
$ for v in '' false 1 0 TRUE nope; do ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=$v \
    npx vitest run --project unit:core core/test/telemetry/tracing_test.ts; done    # 39 passed, each

# Gates
$ npm run lint            # exit 0
$ npm run format:check    # All matched files use Prettier code style!
$ npm run build           # ok
$ npx eslint core/test/telemetry/tracing_test.ts        # clean
$ npx prettier --check core/test/telemetry/tracing_test.ts  # clean

# No production code drifted
$ git diff main --stat -- core/src     # prints nothing

Proof the tests can fail (mutation testing). Coverage is a floor, not proof, so every new test was run against a deliberately broken gate. Five mutations of core/src/telemetry/tracing.ts, each reverted afterwards:

# Mutation Expected to kill Result
A || 'true'|| 'false' unset, '' rows 11 failed (8 matrix + the 3 legacy tests)
B return envValue === 'true' || envValue === '1'return envValue !== 'false' && envValue !== '0' (i.e. implement the docstring) 'TRUE', 'not-a-boolean' rows 8 failed, and no pre-existing test failed
C return true (gate always on) all redaction rows 17 failed (16 matrix + the ungated-invariance test)
D drop the '1' disjunct '1' row 4 failed
E drop the 'true' disjunct 'true', unset, '' rows 16 failed

Every one of the 33 new tests is killed by at least one mutation. Sample failure messages:

# Mutation A
AssertionError: expected '{}' to contain 'test-model'
  ... gate > captures payloads when the variable is unset > traceCallLlm

# Mutation B
AssertionError: expected '{"param1":"value1"}' to be '{}' // Object.is equality
  ... gate > redacts payloads when the variable is 'TRUE' (case matters) > traceToolCall

Mutation B is the important one: it is exactly the behaviour the JSDoc describes, and only the two new rows that pin the case-sensitive implementation catch it. Every mutation was reverted; git diff main --stat -- core/src prints nothing.

Branch coverage of the code under test. shouldAddRequestResponseToSpans and all six gated ternaries are fully covered, verified from the v8 JSON report (no zero-count branch at tracing.ts:122, 148, 189, 228, 247, 309, 407-409). File-level tracing.ts coverage from this suite is 79.32% lines / 75.55% branches; the shortfall is entirely in bindOtelContextToAsyncGenerator / runAsyncGeneratorWithOtelContext / parts of buildLlmRequestForTrace, which are unrelated to this gate and out of scope. vitest.config.ts coverage thresholds were not touched.

Manual End-to-End (E2E) Tests:
No integration or e2e test is added: the gate is a pure process.env read inside the telemetry module with no I/O, no network, and no cross-component contract, so an integration test would only re-exercise the same four functions behind more machinery.

To verify manually, from the repository root:

npm ci && npm run build

# 1. The gate is now pinned in both directions.
npx vitest run --project unit:core core/test/telemetry/tracing_test.ts

# 2. The suite no longer depends on your shell. Each of these must print the
#    same result as step 1 -- on main, the first one fails 3 tests.
ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=false npx vitest run --project unit:core core/test/telemetry/tracing_test.ts
ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=1     npx vitest run --project unit:core core/test/telemetry/tracing_test.ts
ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=nope  npx vitest run --project unit:core core/test/telemetry/tracing_test.ts

# 3. Confirm the tests really bite: break the gate, watch them fail, revert.
sed -i "s/envValue === 'true' || envValue === '1'/envValue !== 'false' \&\& envValue !== '0'/" core/src/telemetry/tracing.ts
npx vitest run --project unit:core core/test/telemetry/tracing_test.ts   # 8 failed
git checkout -- core/src/telemetry/tracing.ts

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits July 30, 2026 05:52
…ESSAGE_CONTENT_IN_SPANS

The suite never stubbed the capture kill switch, so three of its assertions
silently depended on the ambient environment of the machine running them.
On a developer box or CI image that exports
ADK_CAPTURE_MESSAGE_CONTENT_IN_SPANS=false, the gated span attributes collapse
to '{}' and the suite goes red through no fault of the code under test.

Pin the variable to its default in beforeEach and unstub it in afterEach
(vi.restoreAllMocks() does not unstub envs, and unstubEnvs is not enabled in
vitest.config.ts, so this has to be explicit).
…ing gate

shouldAddRequestResponseToSpans() decides whether ADK writes potentially
PII-bearing request/response payloads into OpenTelemetry spans, and had zero
test coverage: all six gated call sites were only ever exercised on the
default capture-ON path.

Add a characterization matrix over the eight env values the implementation
distinguishes, driven entirely through the public tracing entrypoints
(traceCallLlm, traceToolCall, traceMergedToolCalls, traceSendData) so the
module-private helper stays private. The empty-string and 'TRUE' rows are the
load-bearing ones: '' is falsy so it takes the || 'true' default, and the
comparison is case-sensitive so 'TRUE' disables capture. Both pin behaviour
that contradicts the helper's JSDoc; the tests deliberately pin what the code
does today.

One further test asserts the gate only redacts payloads and never suppresses
ungated telemetry.
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