Feat: Port the adk-python OpenTelemetry metrics contract (Part 2/2) - #400
Open
AmaadMartin wants to merge 1 commit into
Open
Conversation
AmaadMartin
force-pushed
the
feat/telemetry-metrics-token-usage-part1
branch
from
July 31, 2026 17:19
2027124 to
22824da
Compare
AmaadMartin
force-pushed
the
feat/telemetry-metrics-token-usage-part2
branch
from
July 31, 2026 17:19
36f6f09 to
32b9524
Compare
The instruments this module declared were adk-js inventions: an agent
duration in milliseconds under gen_ai.agent.invocation.duration, a tool
duration under gen_ai.tool.execution.duration, and a token usage counted
in "1". adk-python emits the GenAI semantic-convention names in seconds
and {token}, with bucket advisories tuned for each, so a dashboard built
against one runtime reads nothing from the other.
Rename and re-unit the four overlapping instruments to the adk-python
spelling and copy their bucket advisories element for element. The three
adk-js-only instruments (request size, response size, workflow steps)
keep their names and behaviour.
adk-python also has gen_ai.invoke_workflow.duration,
gen_ai.invoke_agent.inference_calls and gen_ai.invoke_agent.tool_calls.
Those are not ported here: adk-js has no hook to record them from -- the
first needs a duration and nesting signal in the workflow agents, the
other two a per-invocation counter of model and tool calls -- and an
exported recorder with no caller is dead code. They land with the change
that wires them, from _metrics.py:48-137 and :151-182.
Two behavioural fixes come with the rename:
- The meter was memoised on first use. OTel JS has no proxy meter for
metrics -- unlike tracing -- so a provider registered after the first
recording never saw a measurement. The cache is now keyed on the
provider identity.
- error.type is resolved through resolveErrorType, so a genai ApiError
reports its HTTP status instead of the class name.
Attribute sets follow adk-python exactly, including omitting
gen_ai.response.model until a response has arrived.
Housekeeping that came out of review:
- The module is no longer re-exported from common.ts. It is internal
telemetry plumbing, and export * was publishing every recorder, plus
getElapsedS, as @google/adk API.
- The content byte-sizing helpers move to utils/content_size_utils.ts;
they are not metrics-specific, and their tests move with them. Testing
contentSize directly turned up an unpadded-base64 case the recorder
tests never reached.
- The seven copies of the try/catch/logger.debug wrapper collapse into
safeRecord(), and the recorders take unknown errors, which removes the
`e as Error` cast from all three catch clauses.
- The two client recorders take the single last response they actually
read instead of an array they immediately index.
The unit tests drive a real SDK MeterProvider and assert the exported
data points, which pins the metric names, units, descriptions and bucket
boundaries against the collector rather than against a mock.
AmaadMartin
force-pushed
the
feat/telemetry-metrics-token-usage-part2
branch
from
July 31, 2026 17:28
32b9524 to
e2f69ae
Compare
This was referenced Aug 1, 2026
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
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Closes: N/A
Related: N/A
Problem:
core/src/telemetry/metrics.ts(added by Feat: Implement Opt-in Usage metrics in adk-js #22) emits metric names that adk-python does not emit, in units adk-python does not use:gen_ai.agent.invocation.durationinmswhere adk-python hasgen_ai.invoke_agent.durationins,gen_ai.tool.execution.durationinmswhere adk-python hasgen_ai.execute_tool.durationins, andgen_ai.client.token.usagein1where adk-python uses{token}. It also omits all of the bucket-boundary advisories. A dashboard or OTel backend already consuming adk-python reads nothing from adk-js, which is the whole point of emitting semantic-convention metrics.Two behavioural defects came with it:
metrics.get_meter()proxy re-binds when a provider is installed.MetricsAPI.getMeter()isthis.getMeterProvider().getMeter(...), resolved at call time againstgetGlobal('metrics') || NOOP_METER_PROVIDER. Caching the meter forever means aMeterProviderregistered after the first recording — e.g. an app that runs an agent before callingmaybeSetOtelProviders()— silently never receives a measurement.error.typeiserror.name, so every failed model call reportsApiErrorinstead of the HTTP status that distinguishes a 429 from a 400.Solution: Bring the module onto the adk-python contract, porting
src/google/adk/telemetry/_metrics.pyfield for field._metrics.py:48-137.getMeterProvider()returns the stable no-op singleton and everyrecord*call is a silent no-op — that is how the "opt-in" property is kept, with noif (!provider) returnbranch.error.typethroughresolveErrorType(Part 1), and token counts throughTokenUsage(Part 1).getElapsedS(span, fallbackStartMs), the port of_metrics.py:287-314, and use it at the three existing call sites so every duration comes from one time source.The three adk-js-only instruments (
gen_ai.agent.request.size,gen_ai.agent.response.size,gen_ai.agent.workflow.steps) and theirgetContentSizemachinery are kept exactly as they are — they have no adk-python counterpart, but they are this branch's feature and nothing about the parity port requires removing them.Stacked PR (Part 2/2). Targets
feat/telemetry-metrics-token-usage-part1, which targetsfeat/opt-in-usage-metrics(#22).Collision check.
gh pr list --repo AmaadMartin/adk-js --state open --limit 300returns 299 open PRs;gh pr diff <n> --name-onlyon every plausibly adjacent one (#22, #177, #348, #357, #387, #394) shows only #22 (feat/opt-in-usage-metrics) touching this module. It does not land this change — no semconv names, no bucket advisories, noTokenUsage, noresolveErrorType, no workflow/inference/tool-call instruments — but it owns the same file, so this work is stacked on its branch instead of branched frommain, and this PR is the reconciliation of the two contracts rather than a competing secondmetrics.ts.Scope note. The approved plan for this task said "modules and unit tests only, no call sites". Because the branch this is stacked on has already wired three call sites, keeping the build green required updating them for the changed signatures (
ms->s, the newtoolTypeargument, the options objects). That is the only scope difference, and it is a consequence of stacking rather than a decision to widen the change.Notes for review
_metrics.py— names, units, descriptions, and the attribute valuesgenerate_content,input,output,gemini,vertex_ai.gen_ai.client.operation.durationandgen_ai.client.token.usagecome fromopentelemetry-semantic-conventionsviagen_ai_metrics.create_gen_ai_client_*; their names, units (s,{token}) and descriptions (GenAI operation duration.,Number of input and output tokens used.) are the semconv values, and neither declares bucket boundaries.GEN_AI_AGENT_VERSIONandGEN_AI_TOOL_VERSIONwere deliberately not ported._metrics.py:39-40declares them and nothing reads them; a constant with no reader is dead config.getGoogleLlmVariant(). Python's_guess_gemini_system_name()additionally honours an enterprise-mode env var that adk-js does not model (variant_utils.tsreads onlyGOOGLE_GENAI_USE_VERTEXAI). The emitted strings are byte-identical; only the detection differs, and local convention wins for process-internal behaviour rather than inventing an enterprise-mode env var in adk-js.try/catcharoundgetGoogleLlmVariant()was removed (and with it the test that mocked the util into throwing). It was unreachable defence: every recorder already wraps its whole body intry/catch+logger.debug, so a throw from any dependency is caught one level up. That path is still covered — see "never throws a telemetry failure at the caller", which drives all seven recorders through a provider whosegetMeter()throws and asserts both that nothing propagates and that seven debug logs were emitted. The two provider values it also covered are now pinned byrecords the request and response models(gemini) andreports vertex_ai when GOOGLE_GENAI_USE_VERTEXAI is set, which exercise the real env var throughvi.stubEnvinstead of a module mock.gen_ai.workflow.nestedentirely rather than reportingfalse; an emptyworkflowNameis not emitted (if workflow_name:);recordClientOperationDurationomitsgen_ai.response.modelwhileresponsesis empty, whereasrecordClientTokenUsagealways sets it — it has already returned in the empty case.getElapsedSnarrowsHrTimestructurally, with a smallisHrTimeguard, rather than importingReadableSpanfrom@opentelemetry/sdk-trace-baseinto a runtime position — the APISpantype exposes no timings, but the SDK implementation carries them. Noinstanceof, no cast:TimedSpan extends Spanadds the two fields as optionalunknown. Python'sGenerateContentSpanunwrap has no adk-js counterpart and was dropped.gen_ai.tool.typeat the call site istool.constructor.name, matching both_metrics.py'stool_type(Python passestype(tool).__name__) and the existing adk-js precedent attracing.ts:117, which already sets the same span attribute the same way. This is a display label, not type detection, so the "noconstructor.namefor type detection" rule does not apply.recordWorkflowInvocationDuration,recordInvokeAgentInferenceCallsandrecordInvokeAgentToolCallshave no caller yet. adk-python records them from its workflow-agent and invocation paths, which adk-js does not have an equivalent hook for; wiring them is queued as separate work. They are fully tested here.core/src/common.tsno longer re-exports this module.export * from './telemetry/metrics.js'was publishing every recorder — andgetElapsedS— as@google/adkAPI. These are internal telemetry plumbing, they are called from insidecore/src, and nothing outsidecoreimports them. adk-python keeps the equivalent module private (_metrics.py), and the approved plan for this port says the same.gen_ai.invoke_workflow.duration,gen_ai.invoke_agent.inference_callsandgen_ai.invoke_agent.tool_callsneed a hook adk-js does not have: the first a duration and nesting signal in the workflow agents, the other two a per-invocation counter of model and tool calls (adk-python gets them from itsTelemetryContext). Building that plumbing means changingInvocationContextand the workflow agents, which is a different change from a telemetry port, and an exported recorder with no caller is dead code. They land with the PR that wires them, restored from_metrics.py:48-137(definitions and bucket advisories) and_metrics.py:151-182(recorders); the wiring is queued as its own task. Every instrument in this PR has a live call site.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.
The unit tests no longer mock
@opentelemetry/api. They register a realMeterProviderfrom@opentelemetry/sdk-metricswith an in-memory reader and assert the exported data points, so the metric names, units, descriptions, bucket boundaries, attribute sets and values are checked against what a collector would actually receive. This was also forced by the change: the old file mocked the whole@opentelemetry/apimodule down tometrics.getMeter, which cannot work now that the module resolves the provider (and importstracing.ts, whose module-leveltrace.getTracerwould be undefined under that mock).Cases ported from
adk-python/tests/unittests/telemetry/test_metrics.py(all 8), plus the gaps that file leaves:modelVersion, and with no request model;usageMetadata(asserting thelogger.warnmessage), with only input, with only output, with all-zero counts, and with two responses to prove only the last is read;getElapsedSfor an ended SDK span with exactHrTimetimings (2.25 s to the nanosecond), for a non-recording span with no timings, and for no span at all;metrics.disable()and no provider, all seven recorders run without throwing;getMeter()throws, asserting none of the seven recorders propagates it and each logs once.Three existing assertions were updated, none deleted: the two
recordToolExecutionDurationspies infunctions_test.tsgained thetoolTypeargument, and therecordClientOperationDuration/recordClientTokenUsagespies inllm_agent_test.tsmoved to the options-object form.metrics_e2e_test.tsfollows the two renamed instruments and asserts the newgen_ai.tool.typeattribute. The one removed test is the mockedgetGoogleLlmVariantthrow, covered as described in note 4 above.Coverage.
core/src/telemetry/metrics.tsandcore/src/telemetry/token_usage.tsare both at 100% statements, branches, functions and lines:Proof that the tests can fail. Each was run against mutated source and observed to fail:
0.04->0.05in thegen_ai.execute_tool.durationbucket advisoryinstrument definitions > declares 'gen_ai.execute_tool.duration' …gen_ai.client.token.usageunit{token}->1instrument definitions > declares 'gen_ai.client.token.usage' …expected '1' to be '{token}'if (!cache))meter provider resolution > records against a provider registered after the first call, plus every test that installs its own providerresolveErrorType'sinstanceof ErrorbranchresolveErrorType > falls back to the error name, and 2 moreType suppressions: none. Zero
@ts-expect-error,@ts-ignore,eslint-disable,any,as any,as never,as unknown asor coverage pragmas are added by this PR, insrc/or in tests — including in the test harness, which builds a realMeterProviderand a one-methodApiMeterProviderobject literal rather than casting a partial mock. This part also removes threee as Errorcasts, by typing the recorders'errorparameter asunknown.npm run ts:checkgoes from 308 to 312 errors across the stack, all four incore/test/telemetry/tracing_test.tsand all introduced by Part 1's two new test cases. They are more instances of a defect that already produces eight errors in that file — shared fixtures typed through the built@google/adkdeclarations, passed to a module imported fromcore/src— not a new category, and none of them are insrc/. See Part 1 for the detail.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
npm run build,npm run lint,npm run format:checkandnpm run docs:checkall pass locally.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.
CI status: absent (validated locally)
This is a stacked PR whose base is
feat/telemetry-metrics-token-usage-part1,not
main. The repo's test workflow triggers onpull_request: branches: [main](.github/workflows/validation.yaml:5-6), sono
run-testsjob exists for this PR — the only check that runs isauto-assign, which validates nothing. It was therefore validated locally onthe exact pushed commit
36f6f09f:Revision — complexity review round 1
Everything blocking was addressed; two findings were addressed differently from the suggestion, with reasons.
Done as asked
core/src/common.tsno longer re-exports the module (export *was publishing the recorders andgetElapsedSas@google/adkAPI).core/src/utils/content_size_utils.ts, generically named, with its tests moved tocore/test/utils/content_size_utils_test.ts. TestingcontentSizedirectly immediately paid for itself: the recorder tests never exercised an unpadded base64 payload, so one branch of the padding calculation had never run.metrics_test.tskeeps only the two cases that are about the recorder (the attributes, and that a size is recorded).try/catch/logger.debugcollapsed intosafeRecord(what, record), andcatch (e: unknown)becamecatch (e).e as Erroris gone from all three catch clauses, fixed at the root:resolveErrorTypetakesunknown(Part 1) and the recorders'errorparameters follow.errorType?: unknownis gone. The interface it lived on is gone too; the shape is matched withinnarrowing and the field is honoured only when it is astring.resolveErrorTypemoved tocore/src/utils/error_utils.ts(Part 1).response?: LlmResponseinstead of an array they immediately.at(-1). The "usage is cumulative, only the last chunk counts" rationale moved to the parameter's doc comment, where the caller can see it.Addressed differently
toAttributes()and the usage constants: given a caller, not deleted. The finding was right that nothing in production reached them. But adk-python'stracing.pydoes call the equivalent, and adk-js's hand-rolled substitute was wrong — it reported input tokens frompromptTokenCountalone and output fromcandidatesTokenCountalone. Part 1 now wirestraceCallLlmtoTokenUsage.toAttributes(), which deletes the buggy code instead of the correct code and makes spans emit the cache-read and reasoning attributes. The four constants became module-private, so nothing dead is exported.Not changed, with reasons
getBase64ByteLength->Buffer.byteLength(s, 'base64'). This helper is reachable from the browser bundle —core/src/index_web.tsre-exportscommon.ts, which reachesbase_agent.tsand from there the content sizing — andBufferis not available there. The repo already treats it that way:utils/env_aware_utils.tsguards itsBuffer.frombehindisBrowser(). The arithmetic is kept, now as a two-line function with the constraint written down.getElapsedS(span, fallbackStartMs)keeps the span path. It is_metrics.py:287-314's contract, the two call sites that have an ended span (base_agent,functions) get the span's own start/end rather than a second clock, and the fallback is what the third caller uses. The doc no longer claims a single time source for all callers.AgentEventTally/tallyAgentEventinbase_agent.tsare the base branch's code, unchanged by this PR.logger.warnfor absentusageMetadatamatches_metrics.py:244-249. It fires only when a provider returns no usage at all, which is a real observability gap worth surfacing once rather than hiding at debug level.Re-verified after the revision
Coverage on all four modules —
metrics.ts,token_usage.ts,error_utils.ts,content_size_utils.ts— is 100% statements, branches, functions and lines. The three mutations reported above were re-run against the refactored code and still fail (1, 2 and 26 failures respectively), and a fourth was added for the movedresolveErrorType: deleting itsinstanceof Errorbranch fails 3 of its 8 cases.Revision — complexity review round 2
The one remaining blocking item, B1, is fixed by deletion:
recordWorkflowInvocationDuration,recordInvokeAgentInferenceCallsandrecordInvokeAgentToolCallsare gone, along with the threeHISTOGRAMSentries that existed only for them and theGEN_AI_WORKFLOW_NAME/GEN_AI_WORKFLOW_NESTEDconstants. Their test blocks went with them, and no other test was touched. Every instrument left in this PR has a live call site.Wiring them here was the alternative, and it is a bigger change than the port itself:
gen_ai.invoke_workflow.durationneeds a duration and a nesting signal threaded through the workflow agents, and the two call-count instruments need a per-invocation counter of model and tool calls onInvocationContext. That work is queued separately; the definitions come straight back from_metrics.py:48-137and the recorders from_metrics.py:151-182, so nothing is lost by not carrying them ahead of their callers.Re-verified after the deletion
metrics.ts,token_usage.ts,error_utils.tsandcontent_size_utils.tsare all still at 100% statements, branches, functions and lines, and every row of the mutation table above was re-run against the trimmed module.