ENG-9166 feat(otel): built-in OpenTelemetry trace points, metrics and the reflex-otel instrumentor (1/3) - #6899
ENG-9166 feat(otel): built-in OpenTelemetry trace points, metrics and the reflex-otel instrumentor (1/3)#6899FarhanAliRaza wants to merge 7 commits into
Conversation
…emetry-api >=1.30)
…rt, /ping excluded, shared test fixtures
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2435892c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryAdds built-in OpenTelemetry tracing and metrics to Reflex, plus a separately packaged instrumentor that enables framework trace points and wraps the ASGI application.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the follow-up review scope. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/reflex-base/src/reflex_base/otel.py | Defines the framework’s OpenTelemetry lifecycle, tracing context, event spans, and metric instruments; no eligible follow-up issue remains. |
| packages/reflex-otel/src/reflex_otel/init.py | Implements the OpenTelemetry instrumentor and ASGI middleware factory, with the previously reported uninstrumentation limitation now documented. |
| reflex/app.py | Wraps the built ASGI application when instrumentation is active and records Socket.IO connection and message metrics. |
| packages/reflex-base/src/reflex_base/event/processor/event_processor.py | Wraps enabled event-handler executions in OpenTelemetry spans while preserving the disabled fast path. |
| packages/reflex-base/src/reflex_base/event/context.py | Captures OpenTelemetry context when event contexts are forked for chained-event parenting. |
| packages/reflex-otel/README.md | Documents instrumentation behavior, configuration, metrics, and the accepted ASGI middleware lifecycle limitation. |
Reviews (4): Last reviewed commit: "docs(otel): note the ASGI middleware lif..." | Re-trigger Greptile
b243589 to
54ec281
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Review completed against the latest diff
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/units/reflex_otel/test_init.py">
<violation number="1" location="tests/units/reflex_otel/test_init.py:61">
P3: The first parametrize case `(None, True)` is non-deterministic: when `excluded_urls` is omitted, `ReflexInstrumentor._instrument` falls back to `os.environ.get("OTEL_PYTHON_REFLEX_EXCLUDED_URLS") or os.environ.get("OTEL_PYTHON_EXCLUDED_URLS") or "/ping"`. If either env var is set in the user's or CI environment, `/ping` will not be the effective excluded URL and the assertion `url_disabled("/ping") is True` fails. Clear or monkeypatch both env vars (e.g. `monkeypatch.delenv(..., raising=False)`) around the `instrument()` call to make the test hermetic.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| excluded_urls: str | None, | ||
| ping_disabled: bool, | ||
| ): | ||
| kwargs = {} if excluded_urls is None else {"excluded_urls": excluded_urls} |
There was a problem hiding this comment.
P3: The first parametrize case (None, True) is non-deterministic: when excluded_urls is omitted, ReflexInstrumentor._instrument falls back to os.environ.get("OTEL_PYTHON_REFLEX_EXCLUDED_URLS") or os.environ.get("OTEL_PYTHON_EXCLUDED_URLS") or "/ping". If either env var is set in the user's or CI environment, /ping will not be the effective excluded URL and the assertion url_disabled("/ping") is True fails. Clear or monkeypatch both env vars (e.g. monkeypatch.delenv(..., raising=False)) around the instrument() call to make the test hermetic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/units/reflex_otel/test_init.py, line 61:
<comment>The first parametrize case `(None, True)` is non-deterministic: when `excluded_urls` is omitted, `ReflexInstrumentor._instrument` falls back to `os.environ.get("OTEL_PYTHON_REFLEX_EXCLUDED_URLS") or os.environ.get("OTEL_PYTHON_EXCLUDED_URLS") or "/ping"`. If either env var is set in the user's or CI environment, `/ping` will not be the effective excluded URL and the assertion `url_disabled("/ping") is True` fails. Clear or monkeypatch both env vars (e.g. `monkeypatch.delenv(..., raising=False)`) around the `instrument()` call to make the test hermetic.</comment>
<file context>
@@ -47,3 +47,26 @@ async def app(scope, receive, send): ...
+ excluded_urls: str | None,
+ ping_disabled: bool,
+):
+ kwargs = {} if excluded_urls is None else {"excluded_urls": excluded_urls}
+ instrumentor.instrument(tracer_provider=TracerProvider(), **kwargs)
+ assert otel.asgi_middleware is not None
</file context>
Alek99
left a comment
There was a problem hiding this comment.
Local end-to-end OTel review: browser/backend propagation works on the current lockfile, but these package/runtime issues remain.
| def asgi_middleware(app: otel.ASGIApp) -> otel.ASGIApp: | ||
| return OpenTelemetryMiddleware( | ||
| app, | ||
| excluded_urls=excluded_urls, |
There was a problem hiding this comment.
[P1] Parse excluded_urls for the supported dependency floor
This is version-sensitive: the current lock's ASGI instrumentation accepts strings, but the declared ranges permit API 1.30 with opentelemetry-instrumentation-asgi==0.51b0. I reproduced that version storing the default "/ping" string unchanged; request handling then calls self.excluded_urls.url_disabled(...) and raises AttributeError, so every HTTP/WebSocket request fails once instrumentation wraps the app. Please parse with parse_excluded_urls before constructing the middleware, or raise the lower bound to the first release that accepts strings, and add a minimum-version runtime request test.
| metric_attributes[ATTR_ERROR_TYPE] = type(ex).__qualname__ | ||
| raise | ||
| finally: | ||
| _event_duration.record(perf_counter() - start, metric_attributes) |
There was a problem hiding this comment.
[P2] Record event duration before the event span exits
_event_duration.record() runs after start_as_current_span(...) has exited. With a synchronous span exporter delayed by 80 ms, a roughly 48 μs handler was reported as about 85 ms because exporter time elapsed before the histogram sample was taken. The record also has no current sampled span, so trace-based exemplars lose their trace/span IDs. Move the elapsed-time record into an inner finally while the event span is still current.
Part of #6227 (ENG-9166). Stack: 1/3 (this) → 2/3 compile spans + log correlation → 3/3 browser plugin.
What
Makes the framework instrumentation-aware but zero-cost without the instrumentation package:
reflex-basegets a hard dependency onopentelemetry-api(>=1.30). All trace points live inreflex_base/otel.pybehind a module-levelenabledbool — one attribute read per event when off, no OTel objects created.packages/reflex-otel(reflex_otel.ReflexInstrumentor, aBaseInstrumentorwith the standardopentelemetry_instrumentorentry point) flips the flag.When enabled:
SERVERfor events from the frontend (new trace, or child of the browser span if the event carries atraceparentfield),INTERNALfor chained events, parented under the span that enqueued them via a context snapshot inEventContext.fork(). Attributes:reflex.event.name/txid/parent_txid/background,session.id,code.function.name; exceptions recorded on the span. Events are never parented under the websocket span./pingexcluded, per-message websocket spans off).reflex.event.duration(+error.type),reflex.state.acquire.duration,reflex.websocket.message.size,reflex.websocket.connections.Design
Follows the pattern used by Shiny for Python and pydantic-ai (first-party trace points +
opentelemetry-apino-op) rather than wrapt monkeypatching. Survey of other frameworks and the measured no-op costs are in the ticket.Notes
reflex-otelpinsreflex-base >= <workspace dev version>like the other workspace packages; re-pin at the next release (check_min_deps --check-dev-pinsis the publish gate).publish.yml,dispatch_release.yml,detect.sh.Tests
tests/units/reflex_base/test_otel.py,tests/units/reflex_otel/, otel cases intest_app.py,test_event_processor.py,test_base_state_processor.py; sharedotel_sdkfixture intests/units/conftest.py.