Skip to content

fix(telemetry): break helpers->core import cycle so metrics plugins register#1180

Merged
ajbozarth merged 5 commits into
generative-computing:mainfrom
ajbozarth:fix/helpers-import-cycle
Jun 3, 2026
Merged

fix(telemetry): break helpers->core import cycle so metrics plugins register#1180
ajbozarth merged 5 commits into
generative-computing:mainfrom
ajbozarth:fix/helpers-import-cycle

Conversation

@ajbozarth

@ajbozarth ajbozarth commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Issue

Fixes #1079

Description

mellea/helpers loaded mellea.core at module level, creating a cycle when mellea.core.utils -> mellea.telemetry -> metrics' bottom-of-module registration -> mellea.helpers ran during core init. The except ImportError swallowed the cycle as "plugin framework is not installed", so MELLEA_METRICS_ENABLED=true registered zero plugins in production.

  • helpers/async_helpers.py and openai_compatible_helpers.py: move core imports under TYPE_CHECKING or into function bodies.
  • telemetry/metrics.py: replace except ImportError with explicit _HAS_PLUGIN_FRAMEWORK check; move registration block below __all__.

The lazy-init migration originally scoped in #1079 is declined in favor of eager-parity with the upcoming tracing plugin design.

Testing

  • Tests added to the respective file if code was changed
  • New code has 100% coverage if code was added
  • Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

  • AI coding assistants used

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.

  • Component
  • Requirement
  • Sampling Strategy
  • Tool

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.

@ajbozarth
fix(telemetry): break helpers->core import cycle so metrics plugins r…
1cca89b 
…egister

mellea/helpers loaded mellea.core at module level, creating a cycle when
mellea.core.utils -> mellea.telemetry -> metrics' bottom-of-module
registration -> mellea.helpers ran during core init. The except
ImportError swallowed the cycle as "plugin framework is not installed",
so MELLEA_METRICS_ENABLED=true registered zero plugins in production.

Move helpers' core imports under TYPE_CHECKING or into function bodies,
and replace the except ImportError with an explicit _HAS_PLUGIN_FRAMEWORK
check so future cycle-shape regressions surface loudly.

Closes generative-computing#1079.

Assisted-by: Claude Code
Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
@ajbozarth
ajbozarth requested a review from a team as a code owner June 1, 2026 21:49
@ajbozarth ajbozarth self-assigned this Jun 1, 2026
@ajbozarth
ajbozarth requested review from markstur and planetf1 June 1, 2026 21:49
@github-actions github-actions Bot added the bug Something isn't working label Jun 1, 2026
@ajbozarth

ajbozarth commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

So while wrapping up work on phase 1 of #444 I discovered a chicken and egg issue with registering plugins that makes fully lazy init not work. In addition I found that this bug also affected the tracing work. So I decided to quickly open this bug fix since it was more severe than I thought (essentially metrics has been non-functional since I switched it to use hooks). The remaining items in #1079 are now won't fix

This was referenced Jun 1, 2026
Closed
Merged
planetf1
planetf1 approved these changes Jun 2, 2026
View reviewed changes

@planetf1 planetf1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — reproduced on the parent (0 plugins), confirmed all 8 register here.

Two non-blocking follow-ups:

  • metrics_plugins.py:18 imports from mellea.core.base import GenerationMetadata at module level — the same cycle this PR fixes, safe only because core/__init__.py imports .base before .utils. If that order ever changes the cycle reopens, now silently (since this PR removes the loud failure). Worth a follow-up to make plugin registration lazy and drop the dependency on import order.
  • A regression test in test/package/test_dependency_isolation.py (already runs isolated interpreters) asserting the plugin count is non-zero with metrics enabled would help — the existing plugin tests use mocks, so they couldn't catch this failure mode.

@ajbozarth
test(telemetry): guard metrics plugin registration; harden cycle fix
1ac69ff 
Addresses non-blocking review feedback on generative-computing#1180.

- metrics_plugins.py: move `GenerationMetadata` import out of module level
  (TYPE_CHECKING for the annotation, function-local for the one runtime
  use). Removes the latent cycle that was previously safe only because of
  import order in `mellea/core/__init__.py`.
- test_dependency_isolation.py: add `test_telemetry_metrics_plugins_register`
  that runs in an isolated uv env with `MELLEA_METRICS_ENABLED=true` and
  asserts `plugin_count > 0`. Catches the silent-zero-plugins failure mode
  from generative-computing#1079 that the existing mock-based plugin tests missed. Factor the
  `uv run --isolated` plumbing out of `_run_check` into
  `_run_isolated_script` so both call sites share it.

Assisted-by: Claude Code
Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
@ajbozarth

ajbozarth commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @planetf1
I've updated metrics_plugins.py out of caution despite the fact that ruff theoretically prevents the import order changing.
I also added a new test as you suggested, I previously missed that there was an existing test that I could emulate.
If you could take a second look before I merge this.

@planetf1

planetf1 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

LGTM

@ajbozarth
ajbozarth enabled auto-merge June 3, 2026 16:22
@ajbozarth
ajbozarth added this pull request to the merge queue Jun 3, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 3, 2026
@ajbozarth
ajbozarth added this pull request to the merge queue Jun 3, 2026
Merged via the queue into generative-computing:main with commit a710f3c Jun 3, 2026
9 checks passed
@ajbozarth
ajbozarth deleted the fix/helpers-import-cycle branch June 3, 2026 22:26
ajbozarth added a commit to ajbozarth/mellea that referenced this pull request Jun 8, 2026
@ajbozarth
fix(telemetry): break tracing.py <-> tracing_plugins.py self-cycle
ae0b883 
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>
@abrahamdaniels abrahamdaniels mentioned this pull request Jun 9, 2026
Closed
ajbozarth added a commit to ajbozarth/mellea that referenced this pull request Jun 10, 2026
@ajbozarth
refactor(telemetry)!: move backend tracing onto plugin/hook pattern (g…
bbddd0e 
…enerative-computing#1181)

* refactor(telemetry)!: move backend tracing onto plugin/hook pattern

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>

* fix(telemetry): address PR generative-computing#1181 review feedback

- 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>

* fix(telemetry): break tracing.py <-> tracing_plugins.py self-cycle

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>

* chore(telemetry): clean up review feedback

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>

* fix(telemetry): drop dead detach path that spammed ERROR logs

`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>

* chore(telemetry): clean up tracing tests and docs

- 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>

* fix(telemetry): detach OTel context token after backend span ends

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>

---------

Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@planetf1 planetf1 planetf1 approved these changes
@markstur markstur Awaiting requested review from markstur markstur is a code owner automatically assigned from generative-computing/mellea-maintainers

Assignees

@ajbozarth ajbozarth

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

refactor: migrate metrics to lazy init pattern (and fix auto-register circular-import bug)

2 participants

@ajbozarth @planetf1