Skip to content

fix: reload module for telemetry testing so all tests can run - #805

Merged
jakelorocco merged 6 commits into
mainfrom
jal/telemetry-tests-reload
Apr 13, 2026
Merged

fix: reload module for telemetry testing so all tests can run#805
jakelorocco merged 6 commits into
mainfrom
jal/telemetry-tests-reload

Conversation

@jakelorocco

@jakelorocco jakelorocco commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Misc PR

Type of PR

  • Bug Fix
  • New Feature
  • Documentation
  • Other

Description

  • Link to Issue: Fixes N/A

Allows telemetry tests to always run. The other telemetry tests were already reloading the module. For some reason this one wasn't.

Testing

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

@github-actions

github-actions Bot commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

The PR description has been updated. Please fill out the template for your PR to be reviewed.

@github-actions github-actions Bot added the bug Something isn't working label Apr 9, 2026
@jakelorocco
jakelorocco requested a review from ajbozarth April 9, 2026 18:55
@jakelorocco

Copy link
Copy Markdown
Contributor Author

@ajbozarth, can you please confirm if this change is fine? I don't think I missed any logic here with the telemetry but I also want to make sure I'm not messing up a test that you had a reason for. Thanks!

@ajbozarth

Copy link
Copy Markdown
Contributor

@jakelorocco this is good to merge as is, I had planned on figuring out and fixing this when I got to #444 but it makes sense it was this simple

@ajbozarth
ajbozarth self-requested a review April 9, 2026 21:10

@ajbozarth ajbozarth 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.

actually hold off on merge, I noticed the cli still skipped the test_tracing_backend.py tests and they're failing when run locally

@ajbozarth

Copy link
Copy Markdown
Contributor

A few updates:

The CI skipping is because the tests still contain the old CI skip code, we can remove this, it's handled by the ollama mark and testconf now

if gh_run:
    pytest.skip("Skipping in CI - requires Ollama")

@ajbozarth

Copy link
Copy Markdown
Contributor

Here's the explanation for why the tests are failing now according to Claude:


Good catch on the reload approach — the concept is right but there's a subtle OTel global state issue causing all 6 tests to fail when the full test/telemetry/ suite runs.

When test_tracing.py runs first, its fixtures call trace.set_tracer_provider(provider_1). OTel's global provider is write-once, so when setup_telemetry reloads the module and tries to set a second provider, it's silently rejected:

WARNING opentelemetry.trace:__init__.py:567 Overriding of current TracerProvider is not allowed

tracing._tracer_provider points to the new provider_2, but _backend_tracer was created via trace.get_tracer() which routes through the global provider_1. The span_exporter fixture attaches its InMemorySpanExporter to provider_2, but spans flow through provider_1 — nothing is recorded.

Fix: in mellea/telemetry/tracing.py, bind the tracers to the specific provider instance instead of the OTel global:

# Before
_application_tracer = trace.get_tracer("mellea.application", _mellea_version)
_backend_tracer = trace.get_tracer("mellea.backend", _mellea_version)

# After
_application_tracer = _tracer_provider.get_tracer("mellea.application", _mellea_version)
_backend_tracer = _tracer_provider.get_tracer("mellea.backend", _mellea_version)

@ajbozarth ajbozarth 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.

If you could make that small edit in tracing.py as well as remove the gh_run checks this should work

@planetf1
planetf1 marked this pull request as ready for review April 9, 2026 22:16
@planetf1
planetf1 requested a review from a team as a code owner April 9, 2026 22:16
@planetf1
planetf1 requested review from avinash2692 and markstur April 9, 2026 22:16
@planetf1
planetf1 marked this pull request as draft April 9, 2026 22:17
@planetf1

planetf1 commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

Apologies - had wrong PR open when moving out of draft .

@jakelorocco
jakelorocco force-pushed the jal/telemetry-tests-reload branch from 3abddc3 to fdbd578 Compare April 10, 2026 14:26
@jakelorocco
jakelorocco marked this pull request as ready for review April 10, 2026 15:03
@jakelorocco
jakelorocco requested a review from ajbozarth April 10, 2026 15:03
@jakelorocco

Copy link
Copy Markdown
Contributor Author

Ended up having to make additional changes to the tests; there were two more tests that didn't properly set env vars with the monkeypatched env, so I added those env vars. One of the backend tracing tests had to be updated to support the new tracer changes suggested as well.

@ajbozarth

Copy link
Copy Markdown
Contributor

Updates are looking good to me, here's some nits from Claude, then this is good to go:


Two small issues to flag:

1. Typos in test_metrics.py — the new enable_metrics cleanup deletes wrong env var names:

monkeypatch.delenv("OTL_METRIC_EXPORT_INTERVAL", raising=False)  # should be OTEL_...
monkeypatch.delenv("OTL_SERVICE_NAME", raising=False)             # should be OTEL_...

OTL_ vs OTEL_ — these silently do nothing (raising=False suppresses the error). Compare to clean_metrics_env which correctly uses OTEL_METRIC_EXPORT_INTERVAL and OTEL_SERVICE_NAME.

2. Leftover gh_run parameters — the skip logic was removed from all 6 tests but the gh_run fixture parameter is still in every function signature, e.g.:

async def test_span_duration_captures_async_operation(span_exporter, gh_run):

gh_run is now unused in all of them and should be dropped.


@jakelorocco

Copy link
Copy Markdown
Contributor Author

Resolved remaining issues. Thank you for flagging.

@jakelorocco
jakelorocco added this pull request to the merge queue Apr 13, 2026
Merged via the queue into main with commit 301ca3e Apr 13, 2026
7 checks passed
@jakelorocco
jakelorocco deleted the jal/telemetry-tests-reload branch April 13, 2026 13:40
akihikokuroda pushed a commit to akihikokuroda/mellea that referenced this pull request May 27, 2026
…tive-computing#805)

* fix: reload module for telemetry testing so all tests can run

* fix: remove unecessary ci skip checks

* fix: get tracers from tracing provider

* fix: tracing test and env var setting for tests

* fix: issues with telemetry test changes

* fix: remove unused fixture from telemetry tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants