Migrate LlmUsageAccumulator to typed event filter#138
Merged
chris-colinsky merged 2 commits intoJun 7, 2026
Conversation
Switch the reference accumulator's filter from the 4-stage NodeEvent + namespace + phase + payload-narrow check to a single isinstance(event, LlmCompletionEvent) call. Field access moves from event.pre_state.X to event.usage.X (reusing the same Usage shape the sentinel payload mirrored). Per-invocation bucketing reads event.invocation_id directly off the typed event instead of going through current_invocation_id(), reducing one level of indirection that the typed-event surface enables. Add a defensive guard against a None usage record. Spec types the field as nullable; python's provider always passes a Usage instance today, but the guard keeps the accumulator robust against future providers that exercise the null option. Document the success-only semantic of LlmCompletionEvent and its effect on bucket.call_count: failed LLM calls flow through the exception path and do not emit the typed event, so call_count now reflects successful calls only. Production code migrating an existing accumulator from the sentinel pattern should expect this counting shift if it was previously counting failure-path events. A pipeline tracking attempt-level failure rates needs a separate listener (sentinel NodeEvent pair, or a future failure-event typed variant if that proposal lands). Disclaimer in the docs walkthrough that the captured OTel span name and gen_ai.usage.* attribute family still come from the sentinel handler; the OTel + Langfuse observers have not yet migrated to consuming the typed event. Span names and attribute paths may shift when the observer migration lands. Reframe comment and docs language to describe the architectural state without pinning specific release numbers. Forward-looking promises and historical version pins both age poorly; the CHANGELOG is the authoritative reference for cutoff timing.
There was a problem hiding this comment.
Pull request overview
This PR migrates the LlmUsageAccumulator example and its documentation from the legacy sentinel NodeEvent + payload filtering approach to the typed LlmCompletionEvent observer variant, simplifying filtering and using event.invocation_id for per-invocation bucketing.
Changes:
- Updated
examples/production-observability/main.pyto consumeLlmCompletionEventdirectly and read usage fromevent.usage. - Added/updated documentation explaining the typed-event filter shape, dual-emit transition, and the success-only semantics for
call_count. - Removed legacy sentinel imports (
LLM_NAMESPACE,LlmEventPayload) from the example.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| examples/production-observability/main.py | Switches the accumulator to LlmCompletionEvent, simplifies filtering, and uses event.invocation_id for bucketing. |
| docs/examples/production-observability.md | Updates the walkthrough to match the typed-event accumulator pattern and clarifies migration semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address PR review feedback: the early-return on usage is None was tying call_count to "successful AND reported usage" rather than just "successful." The spec contract has those independent — providers may legitimately omit usage on a successful call. Restructure so the bucket creation and call_count increment happen unconditionally for any LlmCompletionEvent, with the usage-None guard gating only the token-counting math. Successful calls without reported usage now count toward call_count and contribute zero tokens (the only honest value we can record). Smoke test confirms a sequence of one event with usage populated and one event with usage=None produces call_count=2 with token totals matching the populated event only.
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.
Summary
LlmUsageAccumulatorinexamples/production-observability/main.pyfrom the sentinel-namespace +LlmEventPayloadfilter pattern (4 sequential checks) to a singleisinstance(event, LlmCompletionEvent)discriminator. Reads outcome fields directly off the typed event (event.usage.X) and usesevent.invocation_idas the per-invocation key, dropping one level of ContextVar indirection.usage is Noneguard against the nullable spec field shape, and documents the success-only semantic shift onbucket.call_count(failed LLM calls don't emit the typed event, so the counter now reflects successful calls only — production code migrating from the sentinel pattern should expect this).docs/examples/production-observability.mdwalkthrough with the typed-event filter shape, the dual-emit transition framing, thecall_countmigration note, and a disclaimer that the captured OTel span name andgen_ai.usage.*attribute family still come from the sentinel handler (the OTel + Langfuse observers haven't migrated yet — that work is queued behind the request-side fields extension).Test plan
uv run pyright examples/production-observability/main.py(clean)uv run ruff check examples/production-observability/main.py docs/examples/production-observability.md(clean)uv run pytest tests/ -q(1142 pass; no test changes but full suite confirms no regression from the import refactor)__call__confirmed the bucket populates correctly from aLlmCompletionEventand call_count increments per callexamples/production-observability/main.pywith a realLLM_API_KEYand verify the cost-rollup output line still prints with non-zero token countsmain.pyand the matching paragraphs in the docs walkthroughNotes
discuss-llm-completion-event-request-side-fields(spec proposal 0057 will add the request-side fields the observers need). The captured-output samples in the docs reflect the current sentinel-handler behavior; will refresh when that PR lands.LlmCallFailedEventtyped variant would let the accumulator pattern grow attempt-failure tracking. No coord thread for that yet; the proposal-0049 alternative 3 deferred the question pending demand.