Skip to content

Otel changes and testutils#249

Merged
mephenor merged 8 commits into
mainfrom
fix/otel_configuration
Jul 21, 2026
Merged

Otel changes and testutils#249
mephenor merged 8 commits into
mainfrom
fix/otel_configuration

Conversation

@mephenor

@mephenor mephenor commented Jul 21, 2026

Copy link
Copy Markdown
Member

Changed OpenTelemetry config code to manually instrument all installed instrumentation libraries.
This should no longer be used in conjunction with opentelemetry-instrument, which was fragile, either way.

Also removed explicitly setting an env variable to disable OTel as this was no longer working with current OTel versions, as the code evaluating the env variable now runs before our code during autoinstrumentation.
Instead simply skipping TracerProvider creation should serve the same purpose.

Added testutils (and some sanity tests for those) that can be used in the services to build tests around span capture.

Not really sure we need both of the variants, i.e. TracerProvider already exists vs. create a new one.
Might make sense in a unit vs. integration test scenario, where the integration tests hit the normal startup code.

Changes should be effectively non-breaking, so this only bumps the minor version.

@coveralls

coveralls commented Jul 21, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 29833643914

Coverage increased (+0.6%) to 94.275%

Details

  • Coverage increased (+0.6%) from the base build.
  • Patch coverage: 4 uncovered changes across 1 file (79 of 83 lines covered, 95.18%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
src/hexkit/opentelemetry/instrumentation.py 40 36 90.0%
Total (3 files) 83 79 95.18%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 4035
Covered Lines: 3804
Line Coverage: 94.28%
Coverage Strength: 0.94 hits per line

💛 - Coveralls

@mephenor
mephenor requested a review from ckaipf July 21, 2026 11:10
@mephenor
mephenor marked this pull request as ready for review July 21, 2026 11:18
Comment thread src/hexkit/opentelemetry/testutils.py Outdated
Comment thread src/hexkit/opentelemetry/testutils.py
@ckaipf

ckaipf commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

getattr(module, class_name) and .instrument() in the loop aren't guarded. If a pinned instrumentor renames its class in a future bump, getattr raises AttributeError and aborts startup mid-loop. Given the pins it's low-risk, but you may want one instrumentor's failure not to take down the whole loop (catch and log per-entry).

@ckaipf

ckaipf commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

FastAPI (and other class-patching) instrumentation is now order-sensitive. This is the one real behavioral trap. FastAPIInstrumentor().instrument() patches fastapi.FastAPI so only apps instantiated after the call get traced. Under the old opentelemetry-instrument wrapper, patching happened at interpreter startup before any user code, so ordering was guaranteed. Now configure_opentelemetry() must run before the service constructs its FastAPI app, or requests won't be traced.

Co-authored-by: Camill Kaipf <87362681+ckaipf@users.noreply.github.com>
@mephenor

mephenor commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

FastAPI (and other class-patching) instrumentation is now order-sensitive. This is the one real behavioral trap. FastAPIInstrumentor().instrument() patches fastapi.FastAPI so only apps instantiated after the call get traced. Under the old opentelemetry-instrument wrapper, patching happened at interpreter startup before any user code, so ordering was guaranteed. Now configure_opentelemetry() must run before the service constructs its FastAPI app, or requests won't be traced.

I don't think we can avoid that.
Autoinstrumentation via opentelemetry-instrument happens at import time, while everything we have now happens at runtime.
Might be worth a comment, though.

Edit:
See 6aeb4a9

@mephenor

Copy link
Copy Markdown
Member Author

getattr(module, class_name) and .instrument() in the loop aren't guarded. If a pinned instrumentor renames its class in a future bump, getattr raises AttributeError and aborts startup mid-loop. Given the pins it's low-risk, but you may want one instrumentor's failure not to take down the whole loop (catch and log per-entry).

Addressed in dc01a38.

@mephenor
mephenor requested a review from ckaipf July 21, 2026 13:15
@mephenor
mephenor merged commit 64fd883 into main Jul 21, 2026
8 checks passed
@mephenor
mephenor deleted the fix/otel_configuration branch July 21, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants