refactor(telemetry)!: move backend tracing onto plugin/hook pattern#1181
Conversation
Phase 1 of the tracing epic: migrates backend span emission from inline calls scattered across five backends onto a BackendTracingPlugin that subscribes to the existing generation_* hooks for chat and new generation_batch_* hooks for raw. Spans stay live on the OTel context across the API call so nested instrumentation parents under them. - Renames MELLEA_TRACE_* env vars to plural MELLEA_TRACES_* and introduces MELLEA_TRACES_ENABLED umbrella flag, opt-in MELLEA_TRACES_OTLP, and signal-specific OTEL_EXPORTER_OTLP_TRACES_ENDPOINT. No deprecation shim. - Drops deprecated gen_ai.system attribute in favor of gen_ai.provider.name. - Prefixes application-span attributes with mellea. for consistency with backend spans and the existing logging/metrics pillars. - Eagerly initialises the tracer provider and registers the BackendTracingPlugin at module import when MELLEA_TRACES_ENABLED is truthy. Tests reset module state and call _setup_tracing() to re-init after env-var changes, removing the need for importlib.reload. - Adds generation_batch_pre_call/post_call/error hook types so raw-path emits one span per generate_from_raw call (matching OTel GenAI semconv) rather than one per MOT. - Deletes mellea/telemetry/backend_instrumentation.py; backends no longer import from mellea.telemetry (except .context). - Removes the _telemetry_span round-trip on mot._meta and the tracing-specific error block in core/base.py; error-path span closure lives in the plugin's generation_error hook. Closes generative-computing#1045, generative-computing#1046, generative-computing#1047 (Phase 1 of generative-computing#444). BREAKING CHANGE: Public telemetry API surface narrowed: - MELLEA_TRACE_* env vars renamed to plural MELLEA_TRACES_* with no deprecation shim. - The deprecated gen_ai.system span attribute is removed; consumers should read gen_ai.provider.name instead. - The split helpers is_application_tracing_enabled() and is_backend_tracing_enabled() are replaced by a single is_tracing_enabled(). - The application-tracing helpers add_span_event, start_backend_span, end_backend_span, and trace_backend are removed from mellea.telemetry. Backend spans are now emitted by the BackendTracingPlugin automatically; application spans use trace_application (unchanged). - The mellea.telemetry.backend_instrumentation module is deleted along with its exports (start_generate_span, instrument_generate_from_raw, record_token_usage, record_response_metadata, finalize_backend_span). Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
|
Follow-up work spawned by this PR (not part of the tracing epic):
|
|
I also opened #1180 which fixes a bug in metrics that also applies here. (ie that needs to merge first) |
jakelorocco
left a comment
There was a problem hiding this comment.
looks good! I just have a few questions and a few places I think extra comments would be helpful.
- HF backend: detect stop-sequence termination as finish_reason="stop" (previously dropped silently when stopping on a configured stop_string). - Add unit tests for HF post_processing finish_reasons derivation. - Clarify generation_id docstrings/comments to distinguish the Mellea-side hook correlation ID from GenerationMetadata.response_id. - Document why BackendTracingPlugin.on_pre_call runs SEQUENTIAL while the other tracing hooks run FIRE_AND_FORGET. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
|
@jakelorocco I believe 00831a4 addresses all your actionable comments |
planetf1
left a comment
There was a problem hiding this comment.
This PR is a well-structured refactor — moving backend tracing onto the plugin/hook pattern makes it composable and avoids the tight coupling the old backend_instrumentation module had. The OTel GenAI semconv attributes are correct on the happy path. A few issues need addressing, with one blocker around the pre-computed MOT path.
Two tooling findings can't be filed as inline comments since those files aren't in this diff:
tooling/docs-autogen/audit_coverage.py (lines 30–32) — backend_instrumentation was deleted in this PR but is still listed in _CONFIRMED_INTERNAL_MODULES. The comment says this must stay in sync with generate-ast.py, so both need updating. Remove "backend_instrumentation" from the frozenset (leaving just {"json_util"}).
tooling/docs-autogen/generate-ast.py (lines 437–444) — Same deleted module, same stale entry. Remove the "mellea/telemetry/backend_instrumentation" entry and its associated comment block.
|
Overall take: The architectural direction here is right. Moving backend tracing onto the plugin/hook pattern decouples it cleanly from individual backends — new backends get instrumentation for free, and the separation of concerns is much better than the old Two structural issues need fixing before this merges:
The remaining comments (semconv gap, test state isolation, missing export, stale tooling refs) are all quick fixes. None of them block the approach. |
planetf1
left a comment
There was a problem hiding this comment.
Two blocking bugs introduced in this PR, one note referencing a follow-up issue.
|
Leaving a EOW note here for reference, I am still working through triaging all of @planetf1 review above and should have an update by EOD Monday. |
A circular import between mellea/telemetry/tracing.py and mellea/telemetry/tracing_plugins.py silently masqueraded as "plugin framework not installed", leaving BackendTracingPlugin unregistered and backend tracing a no-op when MELLEA_TRACES_ENABLED was set. Mirror the fix shape from PR generative-computing#1180 (metrics-side cycle): - tracing_plugins.py: move span-helper imports out of the module top and into each hook body, breaking the back-reference into tracing.py - tracing.py: replace the swallowing `except ImportError` with the `_HAS_PLUGIN_FRAMEWORK` flag exposed by mellea.plugins.registry, so future import-cycle bugs raise loudly instead of pretending cpex is missing - test_dependency_isolation.py: replace the metrics-only registration test with a unified telemetry-plugins test that asserts both token_metrics and backend_tracing register in an isolated subprocess Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
Independent cleanups from PR generative-computing#1181 review: - mellea/telemetry/__init__.py: re-export `set_span_status_error`. - mellea/telemetry/tracing.py: drop unreachable `_OTEL_AVAILABLE` guard in `finish_backend_span_error`; match `finish_backend_span_success`. - mellea/telemetry/tracing.py: emit `gen_ai.output.type="json_schema"` when structured output is requested (OTel GenAI semconv). - docs/docs/observability/tracing.md: drop "Disabling tracing" section (no parallel in sibling docs). - tooling/docs-autogen: drop stale `backend_instrumentation` entry from `_CONFIRMED_INTERNAL_MODULES`. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
`opentelemetry.context.detach()` swallows exceptions via
`logger.exception("Failed to detach context")` rather than re-raising,
so the local `try/except ValueError` wrapper was unreachable and OTel's
ERROR log fired on every post_call/error hook (FIRE_AND_FORGET, runs on
a different task than pre_call — cross-task detach always fails).
Drop the token plumbing: `_in_flight_spans` stores `Span` directly,
`_detach_token` removed, the token from `attach()` is discarded. The
originating task's context entry is GC'd when that task ends.
`attach()` itself is preserved — it's what parents nested instrumentation
under the backend span.
Assisted-by: Claude Code
Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
Stale tooling refs to |
|
@planetf1 it took a while but I have addressed, responded to, or deferred all you feedback. |
planetf1
left a comment
There was a problem hiding this comment.
Three findings can't anchor to a specific diff line — noted here:
telemetry.md line 90 — "independent" trace scopes: says "Mellea has two independent trace scopes:" — same wording as tracing.md line 103, which is updated inline below. Worth a matching fix: "Mellea has two trace scopes — application and backend — both enabled by MELLEA_TRACES_ENABLED."
Silent env-var rename: a user with MELLEA_TRACE_APPLICATION=true or MELLEA_TRACE_BACKEND=true already set will get no tracing after upgrading with no indication of why. The PR description calls out "no deprecation shim" and that's a fair call for pre-1.0 surface — but a runtime UserWarning when one of the old names is detected, or a note in the troubleshooting FAQ, would save a painful debugging session post-upgrade.
Stale coverage snapshots: docs/metrics/coverage-current.json still lists is_application_tracing_enabled, is_backend_tracing_enabled, trace_backend, and mellea.telemetry.backend_instrumentation — all removed by this PR. No CI gate reads these files so it won't block anything, but a regen would keep the snapshot accurate.
- test_tracing: add OTLP no-endpoint warning and generic-fallback tests; clear _in_flight_spans in the reset helper. - test_tracing_plugins: shutdown the provider before nulling its reference; drop a tautological no-matching-pre_call test that asserted on dict.pop() defaulting; rename the missing-generation-id test to none-generation-id to match the value the test sets. - test_tracing_backend: clear _in_flight_spans in the reset helper. - tracing: trim misleading framing from the _setup_tracer_provider docstring (env-var changes after import need _setup_tracing() re-invoked, not just a setenv). - docs: drop "independent" from "two trace scopes" wording. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
|
Most inline comments addressed (remaining items will be addressed shortly in followup refactor/redesign), for top level review comments:
fixed
As noted this is specifically called out in the BREAKING CHANGE section of the PR description as well as the commit message and will be included in the release changelog as such
IIUC this file is generally out of sync and probably should be removed from version tracking anyway. I don't see reason to regain it in this PR specifically. |
Switch BackendTracingPlugin's post-call and error hooks from FIRE_AND_FORGET to SEQUENTIAL so the finish runs on the originating task. _in_flight_spans stores (Span, Token) again, with detach called inline. Without the detach, sequential backend calls in the same task with no enclosing trace_application span parent under the previous backend span instead of being siblings. Adds test_sequential_backend_calls_produce_siblings. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
|
Pushed Switched the four Added |
planetf1
left a comment
There was a problem hiding this comment.
Two span-lifecycle gaps in the batch paths — details inline. Pre-call hook starts a span before option-merging and formatting (outside the try), and except Exception misses asyncio.CancelledError. Both are localized fixes across the four backends.
One minor advisory in the review body: core/backend.py:103 catches BaseException then awaits the error hook before re-raising — technically interruptible by a double-cancellation race, though the practical likelihood is very low. Mentioning it in case you want to document the assumption with a comment.
planetf1
left a comment
There was a problem hiding this comment.
Superseding the REQUEST_CHANGES — the two batch-path span-lifecycle gaps (pre_call placement and except Exception) are deferred to #1183 which will lift all batch hook firing into the Backend base class. Happy for those to be addressed there. Approving.
bbddd0e
Fixes generative-computing#1182. PR generative-computing#1181 introduced generation_batch_{pre_call,post_call,error} hooks for the raw path; the tracing plugin subscribed but the metrics plugins did not, so generate_from_raw calls produced zero metrics. Token, latency, error, and cost metrics plugins now subscribe to the matching generation_batch_* hook in addition to their existing per-MOT hook. Per-MOT handlers are unchanged. The stale note in docs/docs/observability/metrics.md is removed. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
…nerative-computing#1254) * feat(telemetry): wire metrics plugins to generation_batch_* hooks Fixes generative-computing#1182. PR generative-computing#1181 introduced generation_batch_{pre_call,post_call,error} hooks for the raw path; the tracing plugin subscribed but the metrics plugins did not, so generate_from_raw calls produced zero metrics. Token, latency, error, and cost metrics plugins now subscribe to the matching generation_batch_* hook in addition to their existing per-MOT hook. Per-MOT handlers are unchanged. The stale note in docs/docs/observability/metrics.md is removed. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com> * test(telemetry): add e2e for generation_batch_error metrics path Adds an integration test that drives generate_from_raw against a nonexistent model and asserts the error counter is recorded, covering the dispatch path through the generation_batch_error hook. Also fixes the existing error test to honor OLLAMA_HOST instead of hardcoding localhost:11434, matching the rest of the file. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com> --------- Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
…enerative-computing#1252) * refactor(telemetry): align metrics/pricing init with tracing pattern Wraps init logic in metrics.py and pricing.py in callable `_setup_*()` functions so tests can re-trigger init via a helper instead of `importlib.reload`. Drops the seven module-level env-read constants from metrics.py; env vars are now read at call time inside `_setup_meter_provider()` and hot-path record_* gates check `_meter is None`. Adds test/telemetry/conftest.py with shared `reset_*_state()` helpers that replace ~40 `importlib.reload(mellea.telemetry.*)` calls across all four telemetry test suites. Consolidates three drifted copies of `_reset_tracing_state()` left over from PR generative-computing#1181. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com> * fix(telemetry): null _meter and _tracer fields on disabled-path early return Guarantees the invariant `<field> is not None ⟺ <module>_enabled is True` in `_setup_metrics()` and `_setup_tracing()`. Without this, calling either setup function with a populated state and metrics/tracing now disabled (e.g. via env-var flip) would leave a stale meter/tracer reference that the hot-path `if X is None` gates would treat as live. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com> * test(telemetry): add clean_metrics_env teardown to env-var loop tests Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com> --------- Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
…ase class Closes generative-computing#1183. Folds the additive half of generative-computing#1218 Part 1. - `generate_from_raw` becomes a `@final` wrapper on `Backend` that owns pre/post/error hook firing; backends implement `_generate_from_raw` returning `(results, usage)`. - All five backends drop the inline gen_id, latency timing, and three hook-fire blocks; wrapper catches `BaseException` (matches chat-path generative-computing#1181). - `generation_batch_pre_call` payload mutations now propagate to the backend impl (model_options/format/tool_calls), matching the chat path. Adds `model_options` field to `GenerationBatchPreCallPayload`. Closes a gap from generative-computing#1218 where the batch hook was wired for telemetry observation only. - Standardizes `self._model_id` and `self._provider` on every backend. Inlines model-id resolution into ollama/huggingface `__init__`s and deletes the `_get_*_model_id` helpers; renames `_hf_model_id` to `_model_id`. - Backends with per-MOT token counts (ollama, hf, watsonx) now populate `mot.generation.usage` per MOT; openai/litellm leave it `None` since their APIs only report whole-batch usage. `mot._meta["usage"]` writes preserved for generative-computing#1218's follow-up. - Adds `TestGenerationBatchHookCallSites` mirroring the chat-path tests; updates the custom-backend doc snippet. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
…ase class (generative-computing#1264) * refactor(backends): move generate_from_raw hook firing into Backend base class Closes generative-computing#1183. Folds the additive half of generative-computing#1218 Part 1. - `generate_from_raw` becomes a `@final` wrapper on `Backend` that owns pre/post/error hook firing; backends implement `_generate_from_raw` returning `(results, usage)`. - All five backends drop the inline gen_id, latency timing, and three hook-fire blocks; wrapper catches `BaseException` (matches chat-path generative-computing#1181). - `generation_batch_pre_call` payload mutations now propagate to the backend impl (model_options/format/tool_calls), matching the chat path. Adds `model_options` field to `GenerationBatchPreCallPayload`. Closes a gap from generative-computing#1218 where the batch hook was wired for telemetry observation only. - Standardizes `self._model_id` and `self._provider` on every backend. Inlines model-id resolution into ollama/huggingface `__init__`s and deletes the `_get_*_model_id` helpers; renames `_hf_model_id` to `_model_id`. - Backends with per-MOT token counts (ollama, hf, watsonx) now populate `mot.generation.usage` per MOT; openai/litellm leave it `None` since their APIs only report whole-batch usage. `mot._meta["usage"]` writes preserved for generative-computing#1218's follow-up. - Adds `TestGenerationBatchHookCallSites` mirroring the chat-path tests; updates the custom-backend doc snippet. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com> * fix(plugins): register generation_batch_pre_call payload policy Without an entry in MELLEA_HOOK_PAYLOAD_POLICIES, cpex's default-DENY silently strips plugin mutations to model_options/format/tool_calls on the batch path, even though the new wrapper reads them back. The existing batch mutation test patches invoke_hook entirely, so it never exercises the cpex policy layer; the chat-path equivalent had the same gap. Adds real-plugin tests on both paths that go through register() + invoke_hook so the policy table stays honest. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com> --------- Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
Pull Request
Issue
Fixes #1045, fixes #1046, fixes #1047. Phase 1 of #444. Also closes out item 2 of #909.
Description
Phase 1 of the tracing epic. Migrates backend span emission from inline calls scattered across five backends onto a
BackendTracingPluginthat subscribes to the existinggeneration_*hooks (chat path) and newgeneration_batch_*hooks (raw path). Spans stay live on the OTel context across the API call so nested instrumentation (httpx, langchain) parents under the backend span.Reviewer call-outs:
MELLEA_TRACE_*→MELLEA_TRACES_*(plural), aligned withOTEL_EXPORTER_OTLP_TRACES_ENDPOINTandMELLEA_METRICS_*. AddsMELLEA_TRACES_ENABLEDumbrella flag and opt-inMELLEA_TRACES_OTLP. No deprecation shim — tracing was still pre-1.0 surface.gen_ai.systemremoved in favor ofgen_ai.provider.name. Cleans up the dual emission introduced by feat(telemetry): close five OTel GenAI semantic convention emission gaps (#1035) #1036.generation_batch_pre_call/generation_batch_post_call/generation_batch_errorfor plugin authors. Raw path now emits one span pergenerate_from_rawcall (per OTel GenAI semconv), not one per MOT.ModelOutputThunk.cancel_generation(added upstream by feat(stdlib): add stream_with_chunking() with per-chunk validation (#901) #942 in9e8a9636) had its body rewritten to fireGENERATION_ERRORinstead of calling the removed_telemetry_spanAPI. Reviewers may notice as "extra surface", but it's rebase-driven, not refactor-driven.BREAKING CHANGE: Public telemetry API surface narrowed:
MELLEA_TRACE_*env vars renamed to pluralMELLEA_TRACES_*with no deprecation shim.gen_ai.systemspan attribute is removed; consumers should readgen_ai.provider.nameinstead.is_application_tracing_enabled()andis_backend_tracing_enabled()are replaced by a singleis_tracing_enabled().add_span_event,start_backend_span,end_backend_span, andtrace_backendare removed frommellea.telemetry. Backend spans are now emitted by theBackendTracingPluginautomatically; application spans usetrace_application(unchanged).mellea.telemetry.backend_instrumentationmodule is deleted along with its exports (start_generate_span,instrument_generate_from_raw,record_token_usage,record_response_metadata,finalize_backend_span).Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.