feat!: emit per-chunk streaming span events on backend spans - #1496
feat!: emit per-chunk streaming span events on backend spans#1496ajbozarth wants to merge 5 commits into
Conversation
Add a generic `generation_event` hook fired from `ModelOutputThunk`, plus a `BackendTracingPlugin` handler that records mid-generation span events on the in-flight backend span. Its first consumer is an opt-in `chunk_processed` event, emitted once per streamed chunk when `MELLEA_EMIT_CHUNK_EVENTS` is set, carrying `mellea.chunk_index` and `mellea.chunk_text_length`. The event path is generic and telemetry-agnostic in core: the emit site passes raw domain values, and the plugin maps them to span-event attributes. The gate is checked in core before the hook fires so disabled streaming pays only an env read. Closes generative-computing#1051 Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
Event attribute keys were emitted bare (`chunk_index`, `passed`, ...), clashing with the namespaced span attributes. Prefix each with `mellea.<origin>.<leaf>`, naming the subsystem the value comes from. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
Also make core/utils.py telemetry imports lazy to break a core -> telemetry cycle. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
jakelorocco
left a comment
There was a problem hiding this comment.
The implementation here looks good. I am slightly confused about the end goal of this event / hook (especially given the issue it fixes). Is the end goal of this hook to ultimately replace the StreamEvent type? Otherwise, I'm not quite understanding what the different data dicts might eventually be here.
| emit_chunk_events = self.generation.streaming and _parse_bool_env( | ||
| os.getenv("MELLEA_EMIT_CHUNK_EVENTS", ""), default=False | ||
| ) |
There was a problem hiding this comment.
Is there a performance hit to doing the os.getenv multiple times here? Should we move that specific check into some code that runs once per import?
There was a problem hiding this comment.
I can check, this was in a helper originally so I may have messed that up when I moved this inline
There was a problem hiding this comment.
I looked into this and this is trivial as a performance hit, but I can cache the env var if you're still concerned. It's worth noting this will only run when streaming and only once per astream() call.
| `chunk_processed`: emitted once per streamed chunk during `astream()` | ||
| (opt-in via `MELLEA_EMIT_CHUNK_EVENTS`). `data` keys: | ||
| `chunk_index` (int), `chunk_text_length` (int). |
There was a problem hiding this comment.
I feel that at least for the data types we define; we should have typed implementations even if those types don't necessarily get propagated to the function processing the data.
There was a problem hiding this comment.
My reason for not making data classes is that these events are not intended to be user consumed like Streaming APOI events are. The hooks are available for devs, but their content is essentially telemetry data. I also didn't want to be making a bunch of new individual event hooks either. If this is an issue I'm ok either making event data classes or separate event hooks, I just felt this wants worth it as I saw this as a dev interface compared to streamings user interface
this work is actually agnostic to the new streaming API and is directly on the backend telemetry, though this would nest a few layers under that streaming telemetry
I made this generic because #1073 intends to add more events and would re-use the same hook helper |
planetf1
left a comment
There was a problem hiding this comment.
Went through Bob's review findings against cfca37cf and ran the test suite (66 telemetry + 376 plugin tests pass, ruff/mypy clean). None of the three WARNINGs hold up as blocking, but there's one real issue worth fixing before merge.
WARNING 1 (namespace) — not a bug, don't rename. mellea.generation.chunk_index (backend span, raw provider chunks from astream()) and mellea.streaming.chunk_index (application span, chunking-strategy output units) count different things — one astream() call can span zero or many strategy chunks. Merging them onto one key would conflate incompatible units. Separate namespaces are correct here.
That said, the PR does introduce a real split worth a follow-up: it namespaces span event attributes but leaves existing span attributes bare, so the same span now carries both mellea.full_text_length (attribute) and mellea.streaming.full_text_length (event on that span) for the same value. Not blocking — pre-existing pattern extended, not introduced.
WARNING 2 (negative chunk_text_length) — the mechanism doesn't exist. The emit path is gated on self.generation.streaming, and every streaming backend only ever appends to a str accumulator — there's no mid-stream Pydantic validation that could shrink it (parsing happens once, after the stream ends, under do_set_computed). The suggested guard would also introduce a bug: the else branch sets chunk_text_length = None, which add_span_event filters, so the event would fire with zero attributes.
WARNING 3 (docs) — comparing against the wrong table. The tracing doc documents span events by name only, never by attribute key (see the existing iteration/repair/chunk entries) — the attribute-key table is a different section covering span attributes. The new paragraph matches the existing convention.
What I'd actually change before merge:
MELLEA_EMIT_CHUNK_EVENTSbreaks the established prefix convention — see inline suggestion onmellea/core/base.py.- It's also missing from the env var reference table in
docs/docs/observability/telemetry.md(that file isn't touched by this PR, so no inline suggestion possible there) — add a row once the rename above lands:| `MELLEA_TRACES_CHUNK_EVENTS` | Emit a `chunk_processed` span event per streamed chunk on backend spans | `false` |
Everything else (the ! on commit messages given the acknowledged breaking rename, GenerationEventPayload export, the docstring wording) is a nit — happy to leave those to your judgement.
Rename MELLEA_EMIT_CHUNK_EVENTS to MELLEA_GENERATION_CHUNK_EVENTS and add it to the telemetry env var reference table. The span-event attribute rename this breaking-change note covers landed earlier in db57a90. BREAKING CHANGE: Streaming span-event attribute keys are renamed to the mellea.<origin>.<leaf> namespace (e.g. chunk_index -> mellea.streaming.chunk_index). These keys shipped bare in v0.6.0; trace queries referencing the old names must be updated. No deprecation shim. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
planetf1
left a comment
There was a problem hiding this comment.
A few small follow-ups from re-reviewing the chunk-events changes — none blocking.
Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
|
@jakelorocco @planetf1 I believe I have addressed or responded to all your feedback so far if you could re-review and resolve the threads. |
planetf1
left a comment
There was a problem hiding this comment.
All 4 threads addressed.
- Env var rename — is a better fit than since it gates a hook, not telemetry directly. Proper subsystem prefix.
- Test assertion — Parametrised for both emit on/off, covering both branches. Attribute values pinned in .
- Doc: requires tracing — Pushback correct. The table keeps all vars consistent without annotating tracing dependencies.
- Doc: terminal chunk — Reasonable to omit as implementation detail.
The namespace refactor across all span-event attributes to mellea.* prefixes is solid — prevents OTel reserved key collisions and is architecturally consistent. Tests pass.
Issue
Fixes #1051
Description
Adds streaming span events on the backend generation span (
chat/text_completion), so per-chunk streaming cadence is visible on individual traces rather than only as aggregate metrics.This introduces a reusable event path for backend spans:
generation_eventhook (GenerationEventPayloadcarryingevent_name+ adatadict), fired viaModelOutputThunk._emit_event.BackendTracingPlugin.on_generation_eventhandler that dispatches onevent_nameand records the event on the in-flight backend span via the existingadd_span_event. Future backend-span events reuse this path.Its first consumer is an opt-in
chunk_processedevent, emitted once per streamed chunk duringastream()whenMELLEA_EMIT_CHUNK_EVENTS=true(off by default, since a long response produces one event per chunk). It carriesmellea.generation.chunk_indexandmellea.generation.chunk_text_length.Design notes:
MELLEA_EMIT_CHUNK_EVENTS), checked before the hook fires so the disabled path costs only an env read.on_post_callends the span.Span event attribute keys across the tracing plugins are also namespaced (previously emitted bare, e.g.
chunk_index), followingmellea.<origin>.<leaf>where the origin names the subsystem the value comes from. Note: the streaming-span event attributes were previously released, so renaming them is a breaking change for existing trace queries.Also fixes a latent import-layering bug this surfaced:
mellea/core/utils.pyimportedmellea.telemetryat module scope, forming acore → telemetry → core.basecycle. Those imports are now lazy (function-local), which is behavior-preserving.Open question for review
GenerationEventPayloadcarries an untypeddata: dictrather than a typed event class (contrastStreamEvent, which the streaming hook uses). This keeps core telemetry-agnostic and avoids a payload class per event type. The knownevent_name→datakey contracts are documented in theGenerationEventPayloaddocstring. Still worth confirming before other subscribers depend on it: is the genericdatadict the right call, or should backend events get a typed hierarchy?Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?