-
Notifications
You must be signed in to change notification settings - Fork 18
Bootstrap dormant OpenTelemetry wiring in the Django backend #1583
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,21 +8,14 @@ | |
| from __future__ import annotations | ||
|
|
||
| import contextlib | ||
| import os | ||
| from collections.abc import Iterator | ||
| from typing import Any | ||
|
|
||
| from django.conf import settings | ||
|
|
||
| OTEL_ENDPOINT_ENV = "OTEL_EXPORTER_OTLP_ENDPOINT" | ||
| OTEL_ENABLED_ENV = "OTEL_ENABLED" | ||
| _TRACER_NAME = "consult.worker" | ||
|
|
||
| from otel_common import otel_requested | ||
|
|
||
| def otel_requested() -> bool: | ||
| """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" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This string isn't used anywhere else, just set it on line 41 instead |
||
|
|
||
|
|
||
| def bootstrap_otel(service_name: str) -> None: | ||
|
|
@@ -47,10 +40,9 @@ def bootstrap_otel(service_name: str) -> None: | |
| try: | ||
| configure_otel(service_name=service_name) | ||
| ensure_structlog_otel_processors() | ||
| except RuntimeError: | ||
| except Exception: # noqa: BLE001 - telemetry setup must never block the worker | ||
| logger.warning( | ||
| "OTel endpoint is set but the exporter is unavailable; telemetry " | ||
| "disabled for {service_name}", | ||
| "OTel setup failed; telemetry disabled for {service_name}", | ||
| service_name=service_name, | ||
| ) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| """Shared gating for the OTel bootstraps. | ||
|
|
||
| Kept tiny so the Django and worker entrypoints agree on when telemetry is | ||
| requested and never drift. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import os | ||
|
|
||
| OTEL_ENDPOINT_ENV = "OTEL_EXPORTER_OTLP_ENDPOINT" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we remove string assignments at the top of files that are only used once within the file |
||
| OTEL_ENABLED_ENV = "OTEL_ENABLED" | ||
|
|
||
|
|
||
| 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" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| return enabled and bool(os.environ.get(OTEL_ENDPOINT_ENV)) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| """OpenTelemetry bootstrap for the Django request path. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
|
||
| Dormant unless otel_requested(), so prod keeps running on the existing | ||
| StructuredLogger with no behaviour change. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from otel_common import otel_requested | ||
|
|
||
| _SERVICE_NAME = "consult-backend" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These service names could come from the django env and set in terraform, to save them being defined in code instead |
||
|
|
||
|
|
||
| def configure_django_otel(logger) -> None: | ||
| """Instrument Django and restore the trace-context processor. No-op unless requested.""" | ||
| if not otel_requested(): | ||
| return | ||
|
|
||
| try: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why we need this check? |
||
| from i_dot_ai_utilities.logging._otel import ( | ||
| configure_otel_for_django, | ||
| ensure_structlog_otel_processors, | ||
| ) | ||
| except ImportError: | ||
| logger.warning( | ||
| "OTel endpoint is set but the i-dot-ai-utilities [otel] extra is not " | ||
| "installed; telemetry disabled for {service_name}", | ||
| service_name=_SERVICE_NAME, | ||
| ) | ||
| return | ||
|
|
||
| try: | ||
| # The logger is built at settings import, so restore processors after | ||
| # instrumenting or logs ship without trace ids. | ||
| configure_otel_for_django(service_name=_SERVICE_NAME) | ||
| ensure_structlog_otel_processors() | ||
| except Exception: # noqa: BLE001 - telemetry setup must never block app startup | ||
| logger.warning( | ||
| "OTel setup failed; telemetry disabled for {service_name}", | ||
| service_name=_SERVICE_NAME, | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,39 +5,18 @@ | |
| import pytest | ||
|
|
||
| import otel_bootstrap | ||
| import otel_common | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.: |
||
| monkeypatch.delenv(otel_common.OTEL_ENABLED_ENV, raising=False) | ||
|
|
||
|
|
||
| def _enable(monkeypatch): | ||
| monkeypatch.setenv(otel_bootstrap.OTEL_ENABLED_ENV, "true") | ||
| monkeypatch.setenv(otel_bootstrap.OTEL_ENDPOINT_ENV, "http://collector:4318") | ||
|
|
||
|
|
||
| class TestOtelRequested: | ||
| def test_false_by_default(self): | ||
| assert otel_bootstrap.otel_requested() is False | ||
|
|
||
| def test_false_with_endpoint_but_flag_off(self, monkeypatch): | ||
| monkeypatch.setenv(otel_bootstrap.OTEL_ENDPOINT_ENV, "http://collector:4318") | ||
| assert otel_bootstrap.otel_requested() is False | ||
|
|
||
| def test_false_with_flag_but_no_endpoint(self, monkeypatch): | ||
| monkeypatch.setenv(otel_bootstrap.OTEL_ENABLED_ENV, "true") | ||
| assert otel_bootstrap.otel_requested() is False | ||
|
|
||
| def test_false_when_flag_not_true(self, monkeypatch): | ||
| monkeypatch.setenv(otel_bootstrap.OTEL_ENABLED_ENV, "false") | ||
| monkeypatch.setenv(otel_bootstrap.OTEL_ENDPOINT_ENV, "http://collector:4318") | ||
| assert otel_bootstrap.otel_requested() is False | ||
|
|
||
| def test_true_with_flag_and_endpoint(self, monkeypatch): | ||
| _enable(monkeypatch) | ||
| assert otel_bootstrap.otel_requested() is True | ||
| monkeypatch.setenv(otel_common.OTEL_ENABLED_ENV, "true") | ||
| monkeypatch.setenv(otel_common.OTEL_ENDPOINT_ENV, "http://collector:4318") | ||
|
|
||
|
|
||
| class TestBootstrapOtel: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| """The shared OTel gate stays off unless the flag is on and an endpoint is set.""" | ||
|
|
||
| import pytest | ||
|
|
||
| import otel_common | ||
|
|
||
| ENDPOINT = "http://collector:4317" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The URL for the collector should be an env var |
||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixtures belong in the conftest.py file |
||
| def _clear_otel_env(monkeypatch): | ||
| monkeypatch.delenv(otel_common.OTEL_ENABLED_ENV, raising=False) | ||
| monkeypatch.delenv(otel_common.OTEL_ENDPOINT_ENV, raising=False) | ||
|
|
||
|
|
||
| def test_not_requested_when_env_absent(): | ||
| assert otel_common.otel_requested() is False | ||
|
|
||
|
|
||
| def test_not_requested_when_enabled_without_endpoint(monkeypatch): | ||
| monkeypatch.setenv(otel_common.OTEL_ENABLED_ENV, "true") | ||
| assert otel_common.otel_requested() is False | ||
|
|
||
|
|
||
| def test_not_requested_when_endpoint_without_enabled(monkeypatch): | ||
| monkeypatch.setenv(otel_common.OTEL_ENDPOINT_ENV, ENDPOINT) | ||
| assert otel_common.otel_requested() is False | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("value", ["false", "0", "", "yes"]) | ||
| def test_enabled_flag_requires_true(monkeypatch, value): | ||
| monkeypatch.setenv(otel_common.OTEL_ENABLED_ENV, value) | ||
| monkeypatch.setenv(otel_common.OTEL_ENDPOINT_ENV, ENDPOINT) | ||
| assert otel_common.otel_requested() is False | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("value", ["true", "True ", " TRUE"]) | ||
| def test_enabled_flag_tolerates_case_and_whitespace(monkeypatch, value): | ||
| monkeypatch.setenv(otel_common.OTEL_ENABLED_ENV, value) | ||
| monkeypatch.setenv(otel_common.OTEL_ENDPOINT_ENV, ENDPOINT) | ||
| assert otel_common.otel_requested() is True | ||
|
|
||
|
|
||
| def test_requested_when_enabled_with_endpoint(monkeypatch): | ||
| monkeypatch.setenv(otel_common.OTEL_ENABLED_ENV, "true") | ||
| monkeypatch.setenv(otel_common.OTEL_ENDPOINT_ENV, ENDPOINT) | ||
| assert otel_common.otel_requested() is True | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import json | ||
| import sys | ||
| from unittest.mock import MagicMock | ||
|
|
||
| import pytest | ||
|
|
||
| import otel_django | ||
|
|
||
| ENDPOINT = "http://collector:4317" | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def dormant_env(monkeypatch): | ||
| monkeypatch.delenv("OTEL_ENABLED", raising=False) | ||
| monkeypatch.delenv("OTEL_EXPORTER_OTLP_ENDPOINT", raising=False) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def enabled_env(monkeypatch): | ||
| monkeypatch.setenv("OTEL_ENABLED", "true") | ||
| monkeypatch.setenv("OTEL_EXPORTER_OTLP_ENDPOINT", ENDPOINT) | ||
|
|
||
|
|
||
| def test_configure_is_noop_when_dormant(dormant_env): | ||
| logger = MagicMock() | ||
| otel_django.configure_django_otel(logger) | ||
| logger.warning.assert_not_called() | ||
|
|
||
|
|
||
| def test_configure_warns_when_extra_missing(enabled_env, monkeypatch): | ||
| # The [otel] extra is installed in CI, so fake its absence to reach the ImportError branch. | ||
| monkeypatch.setitem(sys.modules, "i_dot_ai_utilities.logging._otel", None) | ||
| logger = MagicMock() | ||
| otel_django.configure_django_otel(logger) | ||
| logger.warning.assert_called_once() | ||
|
|
||
|
|
||
| def test_enabled_correlates_existing_logs_with_the_active_span(enabled_env, monkeypatch, capsys): | ||
| from i_dot_ai_utilities.logging._otel import setup as otel_setup | ||
| from i_dot_ai_utilities.logging.structured_logger import StructuredLogger | ||
| from i_dot_ai_utilities.logging.types.enrichment_types import ExecutionEnvironmentType | ||
| from i_dot_ai_utilities.logging.types.log_output_format import LogOutputFormat | ||
| from opentelemetry import trace | ||
| from opentelemetry.sdk.trace import TracerProvider | ||
| from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter | ||
|
|
||
| exporter = InMemorySpanExporter() | ||
| monkeypatch.setattr(otel_setup, "_default_otlp_span_exporter", lambda: exporter) | ||
|
|
||
| # Build the logger before configure runs: constructing a StructuredLogger | ||
| # resets structlog config, and configure re-inserts the trace processor after. | ||
| logger = StructuredLogger( | ||
| level="info", | ||
| options={ | ||
| "execution_environment": ExecutionEnvironmentType.LOCAL, | ||
| "log_format": LogOutputFormat.JSON, | ||
| }, | ||
| ) | ||
|
|
||
| otel_setup._reset_for_tests() | ||
| try: | ||
| otel_django.configure_django_otel(logger) | ||
| assert isinstance(trace.get_tracer_provider(), TracerProvider) | ||
|
|
||
| # Drop the parallel request-logging middleware: the existing logger's | ||
| # lines pick up trace ids from whatever span is active, as they would | ||
| # inside a DjangoInstrumentor request span. | ||
| with trace.get_tracer("test").start_as_current_span("request"): | ||
| logger.info("handled request") | ||
|
|
||
| trace.get_tracer_provider().force_flush() | ||
| spans = exporter.get_finished_spans() | ||
| finally: | ||
| otel_setup._reset_for_tests() | ||
|
|
||
| events = [json.loads(line) for line in capsys.readouterr().out.splitlines() if line.startswith("{")] | ||
| logged = next(e for e in events if e.get("message") == "handled request") | ||
|
|
||
| assert len(logged["trace_id"]) == 32 | ||
| assert len(logged["span_id"]) == 16 | ||
|
|
||
| assert spans, "the active span should be recorded and exported" | ||
| assert format(spans[0].context.trace_id, "032x") == logged["trace_id"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate note: can we avoid inline imports, they should be top-level only, unless ABSOLUTELY necessary