Bootstrap dormant OpenTelemetry wiring in the Django backend - #1583
Bootstrap dormant OpenTelemetry wiring in the Django backend#1583Nazehs wants to merge 1 commit into
Conversation
52a4fed to
00d6af5
Compare
|
Claude finished @Nazehs's task in 41s —— View job PR Review: Bootstrap dormant OpenTelemetry wiring in the Django backendClean, well-tested bootstrap. The dormant-by-default gating is solid. A couple of minor points:
⚪️ NIT:
🔵 CONSIDER: broadening to
⚪️ NIT: Add a case to confirm the fallback path. |
00d6af5 to
0c47603
Compare
|
Claude finished @Nazehs's task in 36s —— View job PR Review: Bootstrap dormant OpenTelemetry wiring in the Django backendClean, dormant-by-default OTel bootstrap. Consistent with
⚪️ NIT: Extract to a shared
🔵 CONSIDER: Aligning Otherwise solid — gating, warn-and-continue, test coverage all look good. ✅ |
0c47603 to
41bb41a
Compare
|
Claude finished @Nazehs's task in 41s —— View job PR Review: Bootstrap dormant OpenTelemetry wiring in the Django backendClean, well-structured OTel bootstrap. The dormant-by-default gating is solid. Previous review concerns (shared module,
⚪️ NIT: Move
⚪️ NIT: Test the gating logic once in Otherwise solid. ✅ |
Add an OTel request-path seam that stays a no-op unless OTEL_ENABLED and a collector endpoint are set (dev only via Terraform), so prod keeps running on the existing StructuredLogger with no behaviour change. - otel_django.py gates on env, inserts StructuredLoggingMiddlewareOTel after auth, and configures the tracer provider + structlog processors in ready() - settings expose I_DOT_AI_LOGGER and install the middleware conditionally - warn-and-continue if the [otel] extra or exporter is unavailable
41bb41a to
9bbcc67
Compare
|
Claude finished @Nazehs's task in 31s —— View job PR Review: Bootstrap dormant OpenTelemetry wiring in the Django backend
Clean implementation. Previous review concerns (shared module, No issues to raise. ✅ |
| @@ -14,6 +15,8 @@ def ready(self): | |||
| from otel_bootstrap import bootstrap_otel | |||
There was a problem hiding this comment.
separate note: can we avoid inline imports, they should be top-level only, unless ABSOLUTELY necessary
|
|
||
| def otel_requested() -> bool: | ||
| """Telemetry is requested only when the flag is on and an endpoint is set.""" | ||
| enabled = os.environ.get(OTEL_ENABLED_ENV, "").strip().lower() == "true" |
There was a problem hiding this comment.
Should be able to use the django settings here instead to access the var. Also, don't name env vars with env on the end, it's assumed already
|
|
||
| import os | ||
|
|
||
| OTEL_ENDPOINT_ENV = "OTEL_EXPORTER_OTLP_ENDPOINT" |
There was a problem hiding this comment.
Can we remove string assignments at the top of files that are only used once within the file
| """Bootstrap only when the flag is on and a collector endpoint is configured.""" | ||
| enabled = os.environ.get(OTEL_ENABLED_ENV, "").strip().lower() == "true" | ||
| return enabled and bool(os.environ.get(OTEL_ENDPOINT_ENV)) | ||
| _TRACER_NAME = "consult.worker" |
There was a problem hiding this comment.
This string isn't used anywhere else, just set it on line 41 instead
|
|
||
| from otel_common import otel_requested | ||
|
|
||
| _SERVICE_NAME = "consult-backend" |
There was a problem hiding this comment.
These service names could come from the django env and set in terraform, to save them being defined in code instead
| if not otel_requested(): | ||
| return | ||
|
|
||
| try: |
There was a problem hiding this comment.
I'm not sure why we need this check?
| @@ -0,0 +1,41 @@ | |||
| """OpenTelemetry bootstrap for the Django request path. | |||
There was a problem hiding this comment.
Is there anything in this file that stops it being done as part of the django settings setup instead, similar to how the sentry init is done?
| def _clear_otel_env(monkeypatch): | ||
| monkeypatch.delenv(otel_bootstrap.OTEL_ENDPOINT_ENV, raising=False) | ||
| monkeypatch.delenv(otel_bootstrap.OTEL_ENABLED_ENV, raising=False) | ||
| monkeypatch.delenv(otel_common.OTEL_ENDPOINT_ENV, raising=False) |
There was a problem hiding this comment.
These files are currently the only place in the backend where we have monkeypatch. Could we make use of env vars and patch instead to align with other locations, e.g.:
def _clear_otel_env():
with patch.dict(os.environ, {otel_bootstrap.OTEL_ENDPOINT_ENV: "", otel_bootstrap.OTEL_ENABLED_ENV: ""}, clear=False):
|
|
||
| import otel_common | ||
|
|
||
| ENDPOINT = "http://collector:4317" |
There was a problem hiding this comment.
The URL for the collector should be an env var
| ENDPOINT = "http://collector:4317" | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) |
There was a problem hiding this comment.
Fixtures belong in the conftest.py file
Context
Wires the dormant OTel seam into the Django request path: instruments Django and re-applies the structlog trace-context processor after the logger is built, so the existing request log line picks up trace/span ids. There's no parallel request logger, so nothing gets logged twice.
Changes proposed in this pull request
backend/otel_common.py: sharedotel_requested()gating, so the Django and worker bootstraps can't drift.otel_django.pyandotel_bootstrap.pyboth use it.backend/otel_django.py: when enabled, configures the tracer provider and re-applies the structlog processors inAppConfig.ready()(non-worker only, the worker has its own bootstrap). Warn-and-continue if the[otel]extra is missing or setup fails, so telemetry never blocks startup.otel_bootstrap.pyto the same broad warn-and-continue.trace_id/span_id. Unit tests cover the gating and warn-and-continue.