From e17141093eeed9a597f580bb81557a5a8ed93756 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Mon, 29 Jun 2026 14:33:46 +0100 Subject: [PATCH 01/15] Add OTEL_SDK_DISABLED environment variable to disable sending telemetry --- .env.sample | 5 +++-- docker-compose.yml | 1 + test/.env | 1 + 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.env.sample b/.env.sample index f76700a4..20ec9860 100644 --- a/.env.sample +++ b/.env.sample @@ -118,6 +118,7 @@ HASHER_API_AZ_KEY_VAULT_NAME= # ENV is used by Docker compose to separate runtime environments {dev|test|prod}. ENV= +# set to `false` to enable exporting telemetry +OTEL_SDK_DISABLED=true # gRPC endpoint of the OTel collector, e.g. http://localhost:4317 -# leave empty to disable sending telemetry -OTEL_EXPORTER_OTLP_ENDPOINT=http://localhost:4317 +OTEL_EXPORTER_OTLP_ENDPOINT= diff --git a/docker-compose.yml b/docker-compose.yml index b1457cbc..f3f6b440 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -60,6 +60,7 @@ x-azure-keyvault: &azure-keyvault AZURE_KEY_VAULT_NAME: ${EXPORT_AZ_KEY_VAULT_NAME} x-otel-common: &otel-common + OTEL_SDK_DISABLED: ${OTEL_SDK_DISABLED:-true} OTEL_EXPORTER_OTLP_ENDPOINT: ${OTEL_EXPORTER_OTLP_ENDPOINT:-} OTEL_EXPORTER_OTLP_HEADERS: ${OTEL_EXPORTER_OTLP_HEADERS:-} OTEL_RESOURCE_ATTRIBUTES: "service.namespace=pixl" diff --git a/test/.env b/test/.env index 47754bfb..0210b332 100644 --- a/test/.env +++ b/test/.env @@ -6,6 +6,7 @@ PIXL_QUERY_TIMEOUT=20 CLI_RETRY_SECONDS=90 PIXL_MAX_MESSAGES_IN_FLIGHT=5 TZ=Europe/London +OTEL_SDK_DISABLED=false OTEL_EXPORTER_OTLP_ENDPOINT=http://lgtm:4317 # PIXL PostgreSQL instance From a935b6d162f8bbc8f56a89fedd518199d9e8550c Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Mon, 29 Jun 2026 16:36:57 +0100 Subject: [PATCH 02/15] Manually instrument traces in `pixl_cli`, `orthanc_raw`, and `orthanc_anon` (#644) * Add a configure_tracing function to set up a trace provider * Remove unused OTEL_EXPORTER_OTLP_HEADERS env var * Add a instrument_pika_producer to producer.py to instrument and enrich the producer * Instrument the cli Add a function to get set the relevent otel env vars before calling configure_logging and configure_tracing * Instrument orthanc raw * Instrument orthanc anon * Use logging fixtures from the conftest * Add tests for configure_tracing * Manually create a span before publishing the message This way the span belongs to the CLI rather than to the queue * Remove test of configure_tracing as we're no longer returning a bool * Add OTEL_SDK_DISABLED environment variable to disable sending telemetry --- cli/src/pixl_cli/main.py | 23 +++++++ docker-compose.yml | 1 - orthanc/orthanc-anon/plugin/pixl.py | 47 +++++++++++-- orthanc/orthanc-raw/plugin/pixl.py | 10 ++- pixl_core/src/core/patient_queue/producer.py | 71 +++++++++++++------- pixl_core/src/core/tracing.py | 44 ++++++++++++ pixl_core/tests/conftest.py | 31 +++++++++ pixl_core/tests/test_logging.py | 48 +++---------- pixl_core/tests/test_tracing.py | 63 +++++++++++++++++ 9 files changed, 266 insertions(+), 72 deletions(-) create mode 100644 pixl_core/src/core/tracing.py create mode 100644 pixl_core/tests/test_tracing.py diff --git a/cli/src/pixl_cli/main.py b/cli/src/pixl_cli/main.py index 259a1b12..ec758c2d 100644 --- a/cli/src/pixl_cli/main.py +++ b/cli/src/pixl_cli/main.py @@ -25,8 +25,10 @@ from core.exports import ParquetExport from core.logging import configure_logging from core.patient_queue.producer import PixlProducer +from core.tracing import configure_tracing from decouple import RepositoryEnv, UndefinedValueError from loguru import logger +from opentelemetry.instrumentation.pika import PikaInstrumentor from pixl_cli._config import ( HOST_EXPORT_ROOT_DIR, @@ -51,12 +53,33 @@ os.environ["NO_PROXY"] = os.environ["no_proxy"] = "localhost" +def _configure_telemetry_env_vars() -> None: + """ + Set the OTel environment variables needed by the CLI. + + OTel is configured via environment variables, but the CLI gets its config from a .env in + the current working directory. + + Load the config and set the relevant environment variables. + """ + endpoint = config("OTEL_EXPORTER_OTLP_ENDPOINT", default="") + if not endpoint: + return + + os.environ["OTEL_EXPORTER_OTLP_ENDPOINT"] = endpoint + os.environ["OTEL_SERVICE_NAME"] = "pixl-cli" + os.environ["OTEL_RESOURCE_ATTRIBUTES"] = "service.namespace=pixl" + + @click.group() @click.option("--debug/--no-debug", default=False) def cli(*, debug: bool) -> None: """PIXL command line interface""" logging_level = "DEBUG" if debug else "INFO" + _configure_telemetry_env_vars() configure_logging(level=logging_level) + configure_tracing() + PikaInstrumentor().instrument() cli.add_command(dc) diff --git a/docker-compose.yml b/docker-compose.yml index f3f6b440..ded0743e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -62,7 +62,6 @@ x-azure-keyvault: &azure-keyvault x-otel-common: &otel-common OTEL_SDK_DISABLED: ${OTEL_SDK_DISABLED:-true} OTEL_EXPORTER_OTLP_ENDPOINT: ${OTEL_EXPORTER_OTLP_ENDPOINT:-} - OTEL_EXPORTER_OTLP_HEADERS: ${OTEL_EXPORTER_OTLP_HEADERS:-} OTEL_RESOURCE_ATTRIBUTES: "service.namespace=pixl" OTEL_EXPORTER_OTLP_PROTOCOL: grpc OTEL_LOGS_EXPORTER: none # we define our own loguru sink for exporting logs diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index eca53427..f8a83590 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -37,8 +37,14 @@ from core.exceptions import PixlDiscardError, PixlSkipInstanceError from core.logging import configure_logging from core.project_config.pixl_config_model import load_project_config +from core.tracing import configure_tracing from decouple import config from loguru import logger +from opentelemetry import trace +from opentelemetry.instrumentation.requests import RequestsInstrumentor +from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor +from opentelemetry.propagate import extract +from pixl_dcmd._database import engine as pixl_db_engine from pixl_dcmd.dicom_helpers import get_study_info from pixl_dcmd.main import ( anonymise_dicom_and_update_db, @@ -54,6 +60,7 @@ from typing import Any from core.project_config.pixl_config_model import PixlConfig + from opentelemetry.context import Context from pixl_dcmd.dicom_helpers import StudyInfo ORTHANC_USERNAME = config("ORTHANC_USERNAME") @@ -69,6 +76,15 @@ # Set up logging as main entry point logging_level = config("LOG_LEVEL", default="INFO") configure_logging(level=logging_level) + +# Set up tracing to to correlate logs and traces. +# pixl_dcmd creates its SQLAlchemy engine at import time, so the engine must be +# passed explicitly to the instrumentor +configure_tracing() +SQLAlchemyInstrumentor().instrument(engine=pixl_db_engine) +RequestsInstrumentor().instrument() +tracer = trace.get_tracer("pixl.orthanc_anon") + logger.warning("Running logging at level {}", logging_level) # Set up a thread pool executor for non-blocking calls to Orthanc @@ -220,8 +236,18 @@ def ImportStudiesFromRaw(output, uri, **request): # noqa: ARG001 series_to_keep = payload["SeriesInstanceUIDs"] project_name = payload["ProjectName"] + # Extract the trace context injected into the request headers by the caller, and pass it to + # the thread pool job so the import continues the same trace + headers = {key.lower(): value for key, value in request.get("headers", {}).items()} + parent_context = extract(headers) + executor.submit( - _import_studies_from_raw, study_resource_ids, study_uids, project_name, series_to_keep + _import_studies_from_raw, + study_resource_ids, + study_uids, + project_name, + series_to_keep, + parent_context, ) response = json.dumps({"Message": "Ok"}) @@ -233,6 +259,7 @@ def _import_studies_from_raw( study_uids: list[str], project_name: str, series_to_keep: list[str], + parent_context: Context | None = None, ) -> None: """ Import studies from Orthanc Raw. @@ -240,6 +267,7 @@ def _import_studies_from_raw( Args: study_resource_ids: Resource IDs of the study in Orthanc Raw project_name: Name of the project + parent_context: Trace context extracted from the incoming request, to continue the trace - Pull studies from Orthanc Raw based on its resource ID - Iterate over instances and anonymise them @@ -247,7 +275,11 @@ def _import_studies_from_raw( - Notify the PIXL export-api to send the studies to the relevant endpoint for the project """ - with logger.contextualize(project_name=project_name): + # Continue the trace from the incoming request and bind the project to every log within it. + with ( + tracer.start_as_current_span(name="import_studies_from_raw", context=parent_context), + logger.contextualize(project_name=project_name), + ): anonymised_study_uids = [] for study_resource_id, study_uid in zip(study_resource_ids, study_uids, strict=False): @@ -294,10 +326,13 @@ def _anonymise_study_and_upload( zipped_study_bytes = get_study_zip_archive_from_raw(resource_id=study_resource_id) study_info = _get_study_info_from_first_file(zipped_study_bytes) - with logger.contextualize( - mrn=study_info.mrn, - accession_number=study_info.accession_number, - study_uid=study_info.study_uid, + with ( + tracer.start_as_current_span(name="anonymise_study"), + logger.contextualize( + mrn=study_info.mrn, + accession_number=study_info.accession_number, + study_uid=study_info.study_uid, + ), ): logger.info("Processing project '{}', {}", project_name, study_info) diff --git a/orthanc/orthanc-raw/plugin/pixl.py b/orthanc/orthanc-raw/plugin/pixl.py index cbc716fd..101b1b6a 100644 --- a/orthanc/orthanc-raw/plugin/pixl.py +++ b/orthanc/orthanc-raw/plugin/pixl.py @@ -24,8 +24,10 @@ from typing import TYPE_CHECKING from core.logging import configure_logging +from core.tracing import configure_tracing from decouple import config from loguru import logger +from opentelemetry import trace from pixl_dcmd.tagrecording import record_dicom_headers import orthanc @@ -36,6 +38,11 @@ # Set up logging as main entry point logging_level = config("LOG_LEVEL", default="INFO") configure_logging(level=logging_level) + +# Set up tracing to correlate traces and logs +configure_tracing() +tracer = trace.get_tracer("pixl.orthanc_raw") + logger.warning("Running logging at level {}", logging_level) @@ -48,7 +55,8 @@ def OnHeartBeat(output, uri, **request): # noqa: ARG001 def ReceivedInstanceCallback(receivedDicom: bytes, origin: str) -> Any: # noqa: ARG001 """Optionally record headers from the received DICOM instance.""" if should_record_headers(): - record_dicom_headers(receivedDicom) + with tracer.start_as_current_span(name="record_dicom_headers"): + record_dicom_headers(receivedDicom) return orthanc.ReceivedInstanceAction.KEEP_AS_IS, None diff --git a/pixl_core/src/core/patient_queue/producer.py b/pixl_core/src/core/patient_queue/producer.py index 0218c0d2..78bf21c4 100644 --- a/pixl_core/src/core/patient_queue/producer.py +++ b/pixl_core/src/core/patient_queue/producer.py @@ -17,6 +17,8 @@ from typing import TYPE_CHECKING +from loguru import logger +from opentelemetry import trace from pika import BasicProperties, DeliveryMode from ._base import PixlBlockingInterface @@ -24,7 +26,7 @@ if TYPE_CHECKING: from core.patient_queue.message import Message -from loguru import logger +tracer = trace.get_tracer("pixl_core.patient_queue.producer") class PixlProducer(PixlBlockingInterface): @@ -36,32 +38,49 @@ def publish(self, messages: list[Message], priority: int) -> None: :param messages: list of messages to be sent to queue :param priority: priority of the messages, from 1 (lowest) to 5 (highest) """ - logger.info("Publishing {} messages to queue: {}", len(messages), self.queue_name) - if len(messages) > 0: - for msg in messages: - serialised_msg = msg.serialise() - self._channel.basic_publish( - exchange="", - routing_key=self.queue_name, - body=serialised_msg, - properties=BasicProperties( - delivery_mode=DeliveryMode.Persistent, - priority=priority, - ), - ) - logger.bind( - project_name=msg.project_name, - mrn=msg.mrn, - accession_number=msg.accession_number, - study_uid=msg.study_uid, - ).debug( - "Message {} published to queue {} with priority {}", - msg, - self.queue_name, - priority, - ) - else: + if len(messages) == 0: logger.warning("List of messages is empty so nothing will be published to queue.") + return + + logger.info("Publishing {} messages to queue: {}", len(messages), self.queue_name) + for msg in messages: + attributes = { + "project_name": msg.project_name, + "mrn": msg.mrn, + "accession_number": msg.accession_number, + "study_uid": msg.study_uid, + } + with tracer.start_as_current_span("publish_message", attributes=attributes): + self._publish_message(msg, priority) + + def _publish_message(self, message: Message, priority: int) -> None: + """ + Publish a single serialised message to a queue. + :param message: message to be sent to queue + :param priority: priority of the message, from 1 (lowest) to 5 (highest) + """ + serialised_msg = message.serialise() + self._channel.basic_publish( + exchange="", + routing_key=self.queue_name, + body=serialised_msg, + properties=BasicProperties( + delivery_mode=DeliveryMode.Persistent, + priority=priority, + ), + ) + + logger.bind( + project_name=message.project_name, + mrn=message.mrn, + accession_number=message.accession_number, + study_uid=message.study_uid, + ).debug( + "Message {} published to queue {} with priority {}", + message, + self.queue_name, + priority, + ) def clear_queue(self) -> None: """ diff --git a/pixl_core/src/core/tracing.py b/pixl_core/src/core/tracing.py new file mode 100644 index 00000000..f091000f --- /dev/null +++ b/pixl_core/src/core/tracing.py @@ -0,0 +1,44 @@ +# Copyright (c) University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Configure OpenTelemetry tracing for services not wrapped by opentelemetry-instrument.""" + +from __future__ import annotations + +import atexit +import os + +from opentelemetry import trace +from opentelemetry.exporter.otlp.proto.grpc.trace_exporter import OTLPSpanExporter +from opentelemetry.sdk.resources import Resource +from opentelemetry.sdk.trace import TracerProvider +from opentelemetry.sdk.trace.export import BatchSpanProcessor + +__all__ = ["configure_tracing"] + + +def configure_tracing() -> None: + """ + Set up an OTLP span exporter when OTEL_EXPORTER_OTLP_ENDPOINT is set. + + When the endpoint is not set, tracing and spans are no-ops. + """ + if not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT"): + return + + exporter = OTLPSpanExporter() + processor = BatchSpanProcessor(exporter) + provider = TracerProvider(resource=Resource.create()) + provider.add_span_processor(processor) + trace.set_tracer_provider(provider) + atexit.register(provider.shutdown) diff --git a/pixl_core/tests/conftest.py b/pixl_core/tests/conftest.py index b5675297..ca1e5df0 100644 --- a/pixl_core/tests/conftest.py +++ b/pixl_core/tests/conftest.py @@ -22,12 +22,20 @@ import pytest import requests +from loguru import logger +from opentelemetry.sdk._logs import LoggerProvider +from opentelemetry.sdk._logs.export import ( + InMemoryLogRecordExporter, + SimpleLogRecordProcessor, +) +from opentelemetry.sdk.resources import Resource from pydicom.uid import generate_uid from pytest_pixl.helpers import run_subprocess from sqlalchemy import Engine, create_engine from sqlalchemy.orm import Session, sessionmaker from core.db.models import Base, Extract, Image +from core.logging import OTelSink from core.patient_queue.message import Message if TYPE_CHECKING: @@ -226,3 +234,26 @@ def mock_message() -> Message: "Dec 7 2023 2:08PM", "%b %d %Y %I:%M%p" ).replace(tzinfo=datetime.UTC), ) + + +@pytest.fixture +def log_exporter() -> InMemoryLogRecordExporter: + """In-memory exporter capturing the OTel log records the sink emits.""" + return InMemoryLogRecordExporter() + + +@pytest.fixture +def otel_logger( + monkeypatch: pytest.MonkeyPatch, + log_exporter: InMemoryLogRecordExporter, +) -> Generator[None]: + """Configure an OTelSink using the in-memory exporter.""" + processor = SimpleLogRecordProcessor(log_exporter) + provider = LoggerProvider(resource=Resource.create({"service.name": "test"})) + provider.add_log_record_processor(processor) + monkeypatch.setattr(OTelSink, "_build_provider", lambda _: provider) + + # Set catch=False so loguru doesn't swallow exceptions raised in the sink + handler_id = logger.add(OTelSink(), level="TRACE", catch=False) + yield + logger.remove(handler_id) diff --git a/pixl_core/tests/test_logging.py b/pixl_core/tests/test_logging.py index 79a5774d..5a0293e3 100644 --- a/pixl_core/tests/test_logging.py +++ b/pixl_core/tests/test_logging.py @@ -20,39 +20,11 @@ import pytest from loguru import logger from opentelemetry._logs import SeverityNumber -from opentelemetry.sdk._logs import LoggerProvider -from opentelemetry.sdk._logs.export import ( - InMemoryLogRecordExporter, - SimpleLogRecordProcessor, -) -from opentelemetry.sdk.resources import Resource -from core.logging import OTelSink, configure_logging +from core.logging import configure_logging if TYPE_CHECKING: - from collections.abc import Generator - - -@pytest.fixture -def exporter() -> InMemoryLogRecordExporter: - """In-memory exporter capturing the OTel log records the sink emits.""" - return InMemoryLogRecordExporter() - - -@pytest.fixture -def otel_logger( - exporter: InMemoryLogRecordExporter, - monkeypatch: pytest.MonkeyPatch, -) -> Generator[None]: - """Configure an OTelSink using the in-memory exporter.""" - provider = LoggerProvider(resource=Resource.create({"service.name": "test"})) - provider.add_log_record_processor(SimpleLogRecordProcessor(exporter)) - monkeypatch.setattr(OTelSink, "_build_provider", lambda _: provider) - - # Set catch=False so loguru doesn't swallow exceptions raised in the sink - handler_id = logger.add(OTelSink(), level="TRACE", catch=False) - yield - logger.remove(handler_id) + from opentelemetry.sdk._logs.export import InMemoryLogRecordExporter def test_configure_logging_creates_otel_sink() -> None: @@ -62,23 +34,23 @@ def test_configure_logging_creates_otel_sink() -> None: @pytest.mark.usefixtures("otel_logger") -def test_otel_sink_logs_messages(exporter: InMemoryLogRecordExporter) -> None: +def test_otel_sink_logs_messages(log_exporter: InMemoryLogRecordExporter) -> None: """Test that loguru records are sent to the OTel exporter.""" logger.info("A test message") - record = exporter.get_finished_logs()[0].log_record + record = log_exporter.get_finished_logs()[0].log_record assert record.body == "A test message" @pytest.mark.usefixtures("otel_logger") -def test_bound_fields_become_attributes(exporter: InMemoryLogRecordExporter) -> None: +def test_bound_fields_become_attributes(log_exporter: InMemoryLogRecordExporter) -> None: """Test that bound fields are exported as top-level OTel log attributes.""" logger.bind( project_name="test-project", study_uid="1.2.3", ).info("Processing study.") - record = exporter.get_finished_logs()[0].log_record + record = log_exporter.get_finished_logs()[0].log_record attributes = dict(record.attributes) assert record.body == "Processing study." @@ -90,13 +62,13 @@ def test_bound_fields_become_attributes(exporter: InMemoryLogRecordExporter) -> @pytest.mark.usefixtures("otel_logger") -def test_severity_mapping(exporter: InMemoryLogRecordExporter) -> None: +def test_severity_mapping(log_exporter: InMemoryLogRecordExporter) -> None: """Test loguru levels map correctly to the configured OTel severity name and number.""" logger.trace("Trace message.") logger.info("This is informative.") logger.success("Well done!") - records = [data.log_record for data in exporter.get_finished_logs()] + records = [data.log_record for data in log_exporter.get_finished_logs()] assert [(r.severity_text, r.severity_number) for r in records] == [ ("TRACE", SeverityNumber.TRACE), ("INFO", SeverityNumber.INFO), @@ -105,7 +77,7 @@ def test_severity_mapping(exporter: InMemoryLogRecordExporter) -> None: @pytest.mark.usefixtures("otel_logger") -def test_exception_is_captured(exporter: InMemoryLogRecordExporter) -> None: +def test_exception_is_captured(log_exporter: InMemoryLogRecordExporter) -> None: """ Test that exception type, message and attribute are recorded when calling logger.exception. @@ -120,7 +92,7 @@ def _bad_function() -> None: except ValueError: logger.exception("failed") - record = exporter.get_finished_logs()[0].log_record + record = log_exporter.get_finished_logs()[0].log_record attributes = dict(record.attributes) assert record.severity_text == "ERROR" diff --git a/pixl_core/tests/test_tracing.py b/pixl_core/tests/test_tracing.py new file mode 100644 index 00000000..9e5b1013 --- /dev/null +++ b/pixl_core/tests/test_tracing.py @@ -0,0 +1,63 @@ +# Copyright (c) University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Tests for OpenTelemetry tracing setup and log/trace correlation.""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest +from loguru import logger +from opentelemetry.sdk.trace import TracerProvider +from opentelemetry.sdk.trace.export import SimpleSpanProcessor +from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter + +if TYPE_CHECKING: + from opentelemetry.sdk._logs.export import InMemoryLogRecordExporter + from opentelemetry.trace import Tracer + + +@pytest.fixture +def otel_tracer() -> Tracer: + """A tracer backed by the in-memory span exporter (local, not the global provider).""" + exporter = InMemorySpanExporter() + processor = SimpleSpanProcessor(exporter) + provider = TracerProvider() + provider.add_span_processor(processor) + return provider.get_tracer("test") + + +@pytest.mark.usefixtures("otel_logger") +def test_log_outside_span_has_no_trace_context(log_exporter: InMemoryLogRecordExporter) -> None: + """Test logs emitted with no active span have no trace context.""" + logger.info("no span here") + + record = log_exporter.get_finished_logs()[0].log_record + assert not record.trace_id + assert not record.span_id + + +@pytest.mark.usefixtures("otel_logger") +def test_log_is_correlated_with_active_span( + otel_tracer: Tracer, + log_exporter: InMemoryLogRecordExporter, +) -> None: + """Test logs emitted within a span carry that span's trace_id and span_id.""" + with otel_tracer.start_as_current_span("test_span") as span: + logger.info("inside the span") + span_context = span.get_span_context() + + record = log_exporter.get_finished_logs()[0].log_record + assert record.trace_id == span_context.trace_id + assert record.span_id == span_context.span_id From c66c4d29f21000b5cca7a4b43ad112c426d182e2 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Mon, 29 Jun 2026 16:56:50 +0100 Subject: [PATCH 03/15] Use OTEL_SDK_DISABLED to check whether telemtry collection should be disabled --- cli/src/pixl_cli/main.py | 12 +++++++++++- docs/setup/developer.md | 11 +++++------ pixl_core/src/core/logging.py | 19 ++++++++++++++++--- pixl_core/src/core/tracing.py | 19 +++++++++++++++---- test/conftest.py | 1 + 5 files changed, 48 insertions(+), 14 deletions(-) diff --git a/cli/src/pixl_cli/main.py b/cli/src/pixl_cli/main.py index ec758c2d..091bf62a 100644 --- a/cli/src/pixl_cli/main.py +++ b/cli/src/pixl_cli/main.py @@ -62,10 +62,20 @@ def _configure_telemetry_env_vars() -> None: Load the config and set the relevant environment variables. """ - endpoint = config("OTEL_EXPORTER_OTLP_ENDPOINT", default="") + disabled = config("OTEL_SDK_DISABLED", cast=bool) + if disabled: + os.environ["OTEL_SDK_DISABLED"] = "true" + return + + endpoint = config("OTEL_EXPORTER_OTLP_ENDPOINT") if not endpoint: + logger.warning( + "OTEL_EXPORTER_OTLP_ENDPOINT is not set. Telemetry will not be sent to the collector." + ) + os.environ["OTEL_SDK_DISABLED"] = "true" return + os.environ["OTEL_SDK_DISABLED"] = "false" os.environ["OTEL_EXPORTER_OTLP_ENDPOINT"] = endpoint os.environ["OTEL_SERVICE_NAME"] = "pixl-cli" os.environ["OTEL_RESOURCE_ATTRIBUTES"] = "service.namespace=pixl" diff --git a/docs/setup/developer.md b/docs/setup/developer.md index 14a5c606..7917c71c 100644 --- a/docs/setup/developer.md +++ b/docs/setup/developer.md @@ -139,18 +139,17 @@ PIXL can export structured logs to an PIXL exports telemetry via the OpenTelemetry Protocol (OTLP) and works with any OTel-compatible observability backend. -To enable observability, set `OTEL_EXPORTER_OTLP_ENDPOINT` in the `.env` file to -the gRPC endpoint of an OTel collector, e.g. `localhost:4317` (4317 is the -standard OTLP gRPC port). +To enable observability, set `OTEL_SDK_DISABLED` to `false` and define an `OTEL_EXPORTER_OTLP_ENDPOINT` +in the `.env`. The endpoint be for the gRPC endpoint of an OTel collector, e.g. +`localhost:4317` (4317 is the standard OTLP gRPC port). After starting the PIXL services, logs should start to appear in your collector's UI. ### Disabling OTel -Leave `OTEL_EXPORTER_OTLP_ENDPOINT` empty (or unset) to disable all telemetry. No -other configuration is needed — the services detect the absence of the endpoint -and skip OTel initialisation. +Set `OTEL_SDK_DISABLED` to `true` to disable all telemetry. No other configuration is +needed. ### Adding context to logs diff --git a/pixl_core/src/core/logging.py b/pixl_core/src/core/logging.py index af003968..2b04616f 100644 --- a/pixl_core/src/core/logging.py +++ b/pixl_core/src/core/logging.py @@ -25,6 +25,7 @@ import sys from typing import TYPE_CHECKING +from decouple import config from loguru import logger from opentelemetry._logs import SeverityNumber, set_logger_provider from opentelemetry.exporter.otlp.proto.grpc._log_exporter import OTLPLogExporter @@ -107,6 +108,18 @@ def configure_logging(level: str) -> None: logger.remove() logger.add(sys.stderr, level=level.upper()) - if os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT"): - sink = OTelSink() - logger.add(sink, level=level.upper()) + disabled = config("OTEL_SDK_DISABLED", cast=bool) + if disabled: + logger.debug("OTEL_SDK_DISABLED is set, skipping OTel log configuration") + return + + endpoint = config("OTEL_EXPORTER_OTLP_ENDPOINT") + if not endpoint: + logger.warning( + "OTEL_EXPORTER_OTLP_ENDPOINT is not set. Telemetry will not be sent to the collector." + ) + os.environ["OTEL_SDK_DISABLED"] = "true" + return + + sink = OTelSink() + logger.add(sink, level=level.upper()) diff --git a/pixl_core/src/core/tracing.py b/pixl_core/src/core/tracing.py index f091000f..bf6c84c8 100644 --- a/pixl_core/src/core/tracing.py +++ b/pixl_core/src/core/tracing.py @@ -18,6 +18,8 @@ import atexit import os +from decouple import config +from loguru import logger from opentelemetry import trace from opentelemetry.exporter.otlp.proto.grpc.trace_exporter import OTLPSpanExporter from opentelemetry.sdk.resources import Resource @@ -29,11 +31,20 @@ def configure_tracing() -> None: """ - Set up an OTLP span exporter when OTEL_EXPORTER_OTLP_ENDPOINT is set. - - When the endpoint is not set, tracing and spans are no-ops. + Set up an OTLP span exporter when OTEL_SDK_DISABLED is false + and OTEL_EXPORTER_OTLP_ENDPOINT is set in the environment. """ - if not os.environ.get("OTEL_EXPORTER_OTLP_ENDPOINT"): + disabled = config("OTEL_SDK_DISABLED", cast=bool) + if disabled: + logger.debug("OTEL_SDK_DISABLED is set, skipping OTel log configuration") + return + + endpoint = config("OTEL_EXPORTER_OTLP_ENDPOINT") + if not endpoint: + logger.warning( + "OTEL_EXPORTER_OTLP_ENDPOINT is not set. Telemetry will not be sent to the collector." + ) + os.environ["OTEL_SDK_DISABLED"] = "true" return exporter = OTLPSpanExporter() diff --git a/test/conftest.py b/test/conftest.py index 9d6f3c29..b238a001 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -30,6 +30,7 @@ os.environ["PIXL_DB_USER"] = "pixl_db_username" os.environ["PIXL_DB_PASSWORD"] = "pixl_db_password" os.environ["PIXL_DB_NAME"] = "pixl" +os.environ["OTEL_SDK_DISABLED"] = "false" os.environ["OTEL_EXPORTER_OTLP_ENDPOINT"] = "http://localhost:4317" os.environ["OTEL_RESOURCE_ATTRIBUTES"] = "service.namespace=pixl" os.environ["OTEL_SERVICE_NAME"] = "pixl-cli" From 4a288bfc1c00063560287cd9a6d77715da0cb03f Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 08:22:37 +0100 Subject: [PATCH 04/15] Set OTEL_PYTHON_LOG_AUTO_INSTRUMENTATION to false --- docker-compose.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docker-compose.yml b/docker-compose.yml index ded0743e..8bc57afc 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -64,7 +64,8 @@ x-otel-common: &otel-common OTEL_EXPORTER_OTLP_ENDPOINT: ${OTEL_EXPORTER_OTLP_ENDPOINT:-} OTEL_RESOURCE_ATTRIBUTES: "service.namespace=pixl" OTEL_EXPORTER_OTLP_PROTOCOL: grpc - OTEL_LOGS_EXPORTER: none # we define our own loguru sink for exporting logs + OTEL_PYTHON_LOG_AUTO_INSTRUMENTATION: "false" # we define our own loguru sink for exporting logs + OTEL_LOGS_EXPORTER: none OTEL_TRACES_EXPORTER: otlp OTEL_METRICS_EXPORTER: none From dd04239689af4c8079cdb5f2fe24297884d6aeb1 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 08:36:58 +0100 Subject: [PATCH 05/15] Reuse existing logging provider if it exists --- pixl_core/src/core/logging.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pixl_core/src/core/logging.py b/pixl_core/src/core/logging.py index 2b04616f..eec4f223 100644 --- a/pixl_core/src/core/logging.py +++ b/pixl_core/src/core/logging.py @@ -27,7 +27,7 @@ from decouple import config from loguru import logger -from opentelemetry._logs import SeverityNumber, set_logger_provider +from opentelemetry._logs import SeverityNumber, get_logger_provider, set_logger_provider from opentelemetry.exporter.otlp.proto.grpc._log_exporter import OTLPLogExporter from opentelemetry.sdk._logs import LoggerProvider from opentelemetry.sdk._logs.export import BatchLogRecordProcessor @@ -58,11 +58,18 @@ def __init__(self) -> None: def _build_provider(self) -> LoggerProvider: """ - Create a LoggerProvider for exporting logs via OTLP. + Return LoggerProvider for exporting logs via OTLP. + + Re-use an existing provider if one has already been created. Otherwise, create + a new provider and set it as the global provider. The provider is flushed on exit so we can include logs from short-lived processes, i.e. the CLI. """ + existing_provider = get_logger_provider() + if isinstance(existing_provider, LoggerProvider): + return existing_provider + exporter = OTLPLogExporter() processor = BatchLogRecordProcessor(exporter) provider = LoggerProvider(resource=Resource.create()) From 8b3802af937d3e0a54209a8d4e1dc451f19b7950 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 08:48:49 +0100 Subject: [PATCH 06/15] Remove unused environment variable --- docker-compose.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index 8bc57afc..ded0743e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -64,8 +64,7 @@ x-otel-common: &otel-common OTEL_EXPORTER_OTLP_ENDPOINT: ${OTEL_EXPORTER_OTLP_ENDPOINT:-} OTEL_RESOURCE_ATTRIBUTES: "service.namespace=pixl" OTEL_EXPORTER_OTLP_PROTOCOL: grpc - OTEL_PYTHON_LOG_AUTO_INSTRUMENTATION: "false" # we define our own loguru sink for exporting logs - OTEL_LOGS_EXPORTER: none + OTEL_LOGS_EXPORTER: none # we define our own loguru sink for exporting logs OTEL_TRACES_EXPORTER: otlp OTEL_METRICS_EXPORTER: none From 773188486d2227dfb0d8a9e48416d8475ecc88e1 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 12:15:03 +0100 Subject: [PATCH 07/15] Set OTEL_SDK_DISABLED to true for unit tests --- hasher/tests/conftest.py | 1 + pixl_export/tests/conftest.py | 1 + pixl_imaging/tests/conftest.py | 1 + 3 files changed, 3 insertions(+) diff --git a/hasher/tests/conftest.py b/hasher/tests/conftest.py index e57f9d98..f644891b 100644 --- a/hasher/tests/conftest.py +++ b/hasher/tests/conftest.py @@ -20,6 +20,7 @@ os.environ["LOG_LEVEL"] = "DEBUG" os.environ["AZURE_KEY_VAULT_SECRET_NAME"] = "test-key" os.environ["LOCAL_SALT_VALUE"] = "pixl_salt" +os.environ["OTEL_SDK_DISABLED"] = "true" class MockKeyVault: diff --git a/pixl_export/tests/conftest.py b/pixl_export/tests/conftest.py index fe0c9efe..180c9ad9 100644 --- a/pixl_export/tests/conftest.py +++ b/pixl_export/tests/conftest.py @@ -36,6 +36,7 @@ os.environ["ORTHANC_ANON_USERNAME"] = "orthanc_anon_username" os.environ["ORTHANC_ANON_PASSWORD"] = "orthanc_anon_password" os.environ["ORTHANC_ANON_URL"] = "http://orthanc-anon:8042" +os.environ["OTEL_SDK_DISABLED"] = "true" TEST_DIR = Path(__file__).parent diff --git a/pixl_imaging/tests/conftest.py b/pixl_imaging/tests/conftest.py index ed1c3807..6e745821 100644 --- a/pixl_imaging/tests/conftest.py +++ b/pixl_imaging/tests/conftest.py @@ -42,6 +42,7 @@ os.environ["ORTHANC_PACS_URL"] = "http://localhost:8045" os.environ["ORTHANC_PACS_USERNAME"] = "orthanc" os.environ["ORTHANC_PACS_PASSWORD"] = "orthanc" +os.environ["OTEL_SDK_DISABLED"] = "true" os.environ["PRIMARY_DICOM_SOURCE_MODALITY"] = "UCPRIMARYQR" os.environ["PRIMARY_DICOM_SOURCE_AE_TITLE"] = "PRIMARYQR" os.environ["SECONDARY_DICOM_SOURCE_MODALITY"] = "UCSECONDARYQR" From 6262b02517912ee26fc96428fcc7515d80f111a0 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 12:25:44 +0100 Subject: [PATCH 08/15] Disable the otel sdk in the core unit tests too --- pixl_core/tests/conftest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixl_core/tests/conftest.py b/pixl_core/tests/conftest.py index ca1e5df0..0538f78a 100644 --- a/pixl_core/tests/conftest.py +++ b/pixl_core/tests/conftest.py @@ -69,7 +69,7 @@ os.environ["XNAT_PORT"] = "8080" os.environ["XNAT_DESTINATION"] = "/archive" os.environ["XNAT_OVERWRITE"] = "none" -os.environ["OTEL_EXPORTER_OTLP_ENDPOINT"] = "http://localhost:4317" +os.environ["OTEL_SDK_DISABLED"] = "true" @pytest.fixture(scope="package") From b66196ef965e823e7606ca880527ab22c906a54e Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 13:08:08 +0100 Subject: [PATCH 09/15] Add tests to check provider is reused or created correctly --- pixl_core/tests/conftest.py | 1 + pixl_core/tests/test_logging.py | 25 ++++++++++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/pixl_core/tests/conftest.py b/pixl_core/tests/conftest.py index 0538f78a..324ce88e 100644 --- a/pixl_core/tests/conftest.py +++ b/pixl_core/tests/conftest.py @@ -248,6 +248,7 @@ def otel_logger( log_exporter: InMemoryLogRecordExporter, ) -> Generator[None]: """Configure an OTelSink using the in-memory exporter.""" + monkeypatch.setenv("OTEL_SDK_DISABLED", "false") processor = SimpleLogRecordProcessor(log_exporter) provider = LoggerProvider(resource=Resource.create({"service.name": "test"})) provider.add_log_record_processor(processor) diff --git a/pixl_core/tests/test_logging.py b/pixl_core/tests/test_logging.py index 5a0293e3..a096889b 100644 --- a/pixl_core/tests/test_logging.py +++ b/pixl_core/tests/test_logging.py @@ -20,17 +20,36 @@ import pytest from loguru import logger from opentelemetry._logs import SeverityNumber +from opentelemetry.sdk._logs import LoggerProvider -from core.logging import configure_logging +from core.logging import OTelSink, configure_logging if TYPE_CHECKING: from opentelemetry.sdk._logs.export import InMemoryLogRecordExporter -def test_configure_logging_creates_otel_sink() -> None: - """Test that configure_logging adds the OTel sink when the endpoint is set.""" +def test_build_provider_reuses_existing_provider(monkeypatch: pytest.MonkeyPatch) -> None: + """Test that OTelSink reuses a LoggerProvider already set by opentelemetry-instrument.""" + existing = LoggerProvider() + monkeypatch.setattr("core.logging.get_logger_provider", lambda: existing) + sink = OTelSink() + assert sink.provider is existing + + +def test_configure_logging_skips_otel_when_sdk_disabled() -> None: + """Test that configure_logging only adds stderr when OTEL_SDK_DISABLED is true.""" + configure_logging(level="INFO") + assert len(logger._core.handlers) == 1 # stderr only + logger.remove() + + +def test_configure_logging_creates_otel_sink(monkeypatch: pytest.MonkeyPatch) -> None: + """Test that configure_logging adds the OTel sink when telemetry is enabled.""" + monkeypatch.setenv("OTEL_SDK_DISABLED", "false") + monkeypatch.setenv("OTEL_EXPORTER_OTLP_ENDPOINT", "http://localhost:4317") configure_logging(level="INFO") assert len(logger._core.handlers) == 2 # stderr and OTel sink + logger.remove() @pytest.mark.usefixtures("otel_logger") From 1253d8570f70e495ab9de0f37f390eb4359ec6da Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 13:48:59 +0100 Subject: [PATCH 10/15] Set OTEL_SDK_DISABLED to true for cli unit tests --- cli/tests/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/tests/conftest.py b/cli/tests/conftest.py index 4ff9ffb8..582583c1 100644 --- a/cli/tests/conftest.py +++ b/cli/tests/conftest.py @@ -42,6 +42,7 @@ os.environ[key] = value # Set the remaining environment variables +os.environ["OTEL_SDK_DISABLED"] = "true" os.environ["PROJECT_CONFIGS_DIR"] = str(Path(__file__).parents[2] / "projects/configs") os.environ["EXPORT_AZ_CLIENT_ID"] = "export client id" From 62d565df44e3f92790229af8f748dc39dfdcf870 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 15:52:24 +0100 Subject: [PATCH 11/15] Add a core.telemetry module for configuring otel --- cli/src/pixl_cli/main.py | 28 ++++------ hasher/src/hasher/main.py | 2 +- orthanc/orthanc-anon/plugin/pixl.py | 3 +- orthanc/orthanc-raw/plugin/pixl.py | 3 +- pixl_core/src/core/logging.py | 35 +------------ .../src/core/{tracing.py => telemetry.py} | 52 ++++++++++++++++--- pixl_export/src/pixl_export/main.py | 2 +- pixl_imaging/src/pixl_imaging/main.py | 2 +- 8 files changed, 61 insertions(+), 66 deletions(-) rename pixl_core/src/core/{tracing.py => telemetry.py} (62%) diff --git a/cli/src/pixl_cli/main.py b/cli/src/pixl_cli/main.py index 091bf62a..492cfe1b 100644 --- a/cli/src/pixl_cli/main.py +++ b/cli/src/pixl_cli/main.py @@ -23,9 +23,8 @@ import click import requests from core.exports import ParquetExport -from core.logging import configure_logging from core.patient_queue.producer import PixlProducer -from core.tracing import configure_tracing +from core.telemetry import configure_logging, configure_tracing, telemetry_is_enabled from decouple import RepositoryEnv, UndefinedValueError from loguru import logger from opentelemetry.instrumentation.pika import PikaInstrumentor @@ -53,7 +52,7 @@ os.environ["NO_PROXY"] = os.environ["no_proxy"] = "localhost" -def _configure_telemetry_env_vars() -> None: +def _configure_telemetry(logging_level: str) -> None: """ Set the OTel environment variables needed by the CLI. @@ -62,34 +61,25 @@ def _configure_telemetry_env_vars() -> None: Load the config and set the relevant environment variables. """ - disabled = config("OTEL_SDK_DISABLED", cast=bool) - if disabled: - os.environ["OTEL_SDK_DISABLED"] = "true" - return - - endpoint = config("OTEL_EXPORTER_OTLP_ENDPOINT") - if not endpoint: - logger.warning( - "OTEL_EXPORTER_OTLP_ENDPOINT is not set. Telemetry will not be sent to the collector." - ) - os.environ["OTEL_SDK_DISABLED"] = "true" + if not telemetry_is_enabled(): return os.environ["OTEL_SDK_DISABLED"] = "false" - os.environ["OTEL_EXPORTER_OTLP_ENDPOINT"] = endpoint + os.environ["OTEL_EXPORTER_OTLP_ENDPOINT"] = config("OTEL_EXPORTER_OTLP_ENDPOINT") os.environ["OTEL_SERVICE_NAME"] = "pixl-cli" os.environ["OTEL_RESOURCE_ATTRIBUTES"] = "service.namespace=pixl" + configure_logging(level=logging_level) + configure_tracing() + PikaInstrumentor().instrument() + @click.group() @click.option("--debug/--no-debug", default=False) def cli(*, debug: bool) -> None: """PIXL command line interface""" logging_level = "DEBUG" if debug else "INFO" - _configure_telemetry_env_vars() - configure_logging(level=logging_level) - configure_tracing() - PikaInstrumentor().instrument() + _configure_telemetry(logging_level=logging_level) cli.add_command(dc) diff --git a/hasher/src/hasher/main.py b/hasher/src/hasher/main.py index d8069f59..c2c87a8b 100644 --- a/hasher/src/hasher/main.py +++ b/hasher/src/hasher/main.py @@ -15,7 +15,7 @@ from __future__ import annotations -from core.logging import configure_logging +from core.telemetry import configure_logging from decouple import config # type: ignore [import-untyped] from fastapi import FastAPI from fastapi.middleware.cors import CORSMiddleware diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index f8a83590..8d1c5b04 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -35,9 +35,8 @@ import pydicom import requests from core.exceptions import PixlDiscardError, PixlSkipInstanceError -from core.logging import configure_logging from core.project_config.pixl_config_model import load_project_config -from core.tracing import configure_tracing +from core.telemetry import configure_logging, configure_tracing from decouple import config from loguru import logger from opentelemetry import trace diff --git a/orthanc/orthanc-raw/plugin/pixl.py b/orthanc/orthanc-raw/plugin/pixl.py index 101b1b6a..55280186 100644 --- a/orthanc/orthanc-raw/plugin/pixl.py +++ b/orthanc/orthanc-raw/plugin/pixl.py @@ -23,8 +23,7 @@ import os from typing import TYPE_CHECKING -from core.logging import configure_logging -from core.tracing import configure_tracing +from core.telemetry import configure_logging, configure_tracing from decouple import config from loguru import logger from opentelemetry import trace diff --git a/pixl_core/src/core/logging.py b/pixl_core/src/core/logging.py index eec4f223..fc62f4b7 100644 --- a/pixl_core/src/core/logging.py +++ b/pixl_core/src/core/logging.py @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. """ -Configure loguru to emit structured logs to an OpenTelemetry collector via OTLP. +Provides a loguru sink that forwards records to an OpenTelemetry collector via OTLP. Note, only loguru records are exported. Logs from third-party libraries are not forwarded to the OTel collector. @@ -21,12 +21,8 @@ from __future__ import annotations import atexit -import os -import sys from typing import TYPE_CHECKING -from decouple import config -from loguru import logger from opentelemetry._logs import SeverityNumber, get_logger_provider, set_logger_provider from opentelemetry.exporter.otlp.proto.grpc._log_exporter import OTLPLogExporter from opentelemetry.sdk._logs import LoggerProvider @@ -36,7 +32,7 @@ if TYPE_CHECKING: from loguru import Message -__all__ = ["configure_logging"] +__all__ = ["OTelSink"] # Map loguru level names to OTel severity LOGURU_TO_OTEL: dict[str, SeverityNumber] = { @@ -103,30 +99,3 @@ def __call__(self, message: Message) -> None: attributes=attributes, exception=exception.value if exception else None, ) - - -def configure_logging(level: str) -> None: - """ - Configure loguru for a PIXL service. - - Always logs to stderr, which will be viewable in the Docker logs. When - OTEL_EXPORTER_OTLP_ENDPOINT is set, also send logs to the OTel collector. - """ - logger.remove() - logger.add(sys.stderr, level=level.upper()) - - disabled = config("OTEL_SDK_DISABLED", cast=bool) - if disabled: - logger.debug("OTEL_SDK_DISABLED is set, skipping OTel log configuration") - return - - endpoint = config("OTEL_EXPORTER_OTLP_ENDPOINT") - if not endpoint: - logger.warning( - "OTEL_EXPORTER_OTLP_ENDPOINT is not set. Telemetry will not be sent to the collector." - ) - os.environ["OTEL_SDK_DISABLED"] = "true" - return - - sink = OTelSink() - logger.add(sink, level=level.upper()) diff --git a/pixl_core/src/core/tracing.py b/pixl_core/src/core/telemetry.py similarity index 62% rename from pixl_core/src/core/tracing.py rename to pixl_core/src/core/telemetry.py index bf6c84c8..5951c14e 100644 --- a/pixl_core/src/core/tracing.py +++ b/pixl_core/src/core/telemetry.py @@ -11,12 +11,13 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -"""Configure OpenTelemetry tracing for services not wrapped by opentelemetry-instrument.""" +"""Configure OpenTelemetry for PIXL services.""" from __future__ import annotations import atexit import os +import sys from decouple import config from loguru import logger @@ -26,25 +27,62 @@ from opentelemetry.sdk.trace import TracerProvider from opentelemetry.sdk.trace.export import BatchSpanProcessor -__all__ = ["configure_tracing"] +from core.logging import OTelSink +__all__ = [ + "configure_logging", + "configure_tracing", + "telemetry_is_enabled", +] -def configure_tracing() -> None: + +def telemetry_is_enabled() -> bool: """ - Set up an OTLP span exporter when OTEL_SDK_DISABLED is false - and OTEL_EXPORTER_OTLP_ENDPOINT is set in the environment. + Check whether telemetry should be enabled. + + It should be disabled if OTEL_SDK_DISABLED is true or if OTEL_EXPORTER_OTLP_ENDPOINT is not set. """ disabled = config("OTEL_SDK_DISABLED", cast=bool) if disabled: - logger.debug("OTEL_SDK_DISABLED is set, skipping OTel log configuration") - return + logger.debug("OTEL_SDK_DISABLED is set, skipping OTel configuration") + return False endpoint = config("OTEL_EXPORTER_OTLP_ENDPOINT") if not endpoint: logger.warning( "OTEL_EXPORTER_OTLP_ENDPOINT is not set. Telemetry will not be sent to the collector." ) + # Disable the SDK to avoid errors from the OTel SDK os.environ["OTEL_SDK_DISABLED"] = "true" + return False + + return True + + +def configure_logging(level: str) -> None: + """ + Configure loguru for a PIXL service. + + Always logs to stderr, which will be viewable in the Docker logs. When + OTEL_SDK_DISABLED is false and OTEL_EXPORTER_OTLP_ENDPOINT is set, also + sends logs to the OTel collector. + """ + logger.remove() + logger.add(sys.stderr, level=level.upper()) + + if not telemetry_is_enabled(): + return + + sink = OTelSink() + logger.add(sink, level=level.upper()) + + +def configure_tracing() -> None: + """ + Set up an OTLP span exporter when OTEL_SDK_DISABLED is false + and OTEL_EXPORTER_OTLP_ENDPOINT is set in the environment. + """ + if not telemetry_is_enabled(): return exporter = OTLPSpanExporter() diff --git a/pixl_export/src/pixl_export/main.py b/pixl_export/src/pixl_export/main.py index 39742f85..1b25781f 100644 --- a/pixl_export/src/pixl_export/main.py +++ b/pixl_export/src/pixl_export/main.py @@ -23,7 +23,7 @@ from typing import Annotated from core.exports import ParquetExport -from core.logging import configure_logging +from core.telemetry import configure_logging from core.rest_api.router import router from core.uploader import get_uploader from decouple import config # type: ignore [import-untyped] diff --git a/pixl_imaging/src/pixl_imaging/main.py b/pixl_imaging/src/pixl_imaging/main.py index 751aa449..245dffe3 100644 --- a/pixl_imaging/src/pixl_imaging/main.py +++ b/pixl_imaging/src/pixl_imaging/main.py @@ -18,7 +18,7 @@ import asyncio import importlib.metadata -from core.logging import configure_logging +from core.telemetry import configure_logging from core.patient_queue.subscriber import PixlConsumer from core.rest_api.router import router, state from decouple import config From 3d8fb96e47afea9b2b606f81597c59abdcd7b476 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 15:54:56 +0100 Subject: [PATCH 12/15] Import configure_logging and configure_tracing from the new telemetry module --- pixl_core/tests/test_logging.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pixl_core/tests/test_logging.py b/pixl_core/tests/test_logging.py index a096889b..a8b7f76a 100644 --- a/pixl_core/tests/test_logging.py +++ b/pixl_core/tests/test_logging.py @@ -22,7 +22,8 @@ from opentelemetry._logs import SeverityNumber from opentelemetry.sdk._logs import LoggerProvider -from core.logging import OTelSink, configure_logging +from core.logging import OTelSink +from core.telemetry import configure_logging if TYPE_CHECKING: from opentelemetry.sdk._logs.export import InMemoryLogRecordExporter From cfa44e3a6dad917e8c2af0c23c97af013e359334 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Tue, 30 Jun 2026 16:00:12 +0100 Subject: [PATCH 13/15] Add missing docstring --- pixl_core/src/core/logging.py | 1 + pixl_export/src/pixl_export/main.py | 2 +- pixl_imaging/src/pixl_imaging/main.py | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pixl_core/src/core/logging.py b/pixl_core/src/core/logging.py index fc62f4b7..5727dca6 100644 --- a/pixl_core/src/core/logging.py +++ b/pixl_core/src/core/logging.py @@ -50,6 +50,7 @@ class OTelSink: """Send loguru records to an OTel logs exporter via OTLP.""" def __init__(self) -> None: + """Initialise the OTel sink.""" self.provider = self._build_provider() def _build_provider(self) -> LoggerProvider: diff --git a/pixl_export/src/pixl_export/main.py b/pixl_export/src/pixl_export/main.py index 1b25781f..fc7b8554 100644 --- a/pixl_export/src/pixl_export/main.py +++ b/pixl_export/src/pixl_export/main.py @@ -23,8 +23,8 @@ from typing import Annotated from core.exports import ParquetExport -from core.telemetry import configure_logging from core.rest_api.router import router +from core.telemetry import configure_logging from core.uploader import get_uploader from decouple import config # type: ignore [import-untyped] from fastapi import Body, FastAPI, HTTPException diff --git a/pixl_imaging/src/pixl_imaging/main.py b/pixl_imaging/src/pixl_imaging/main.py index 245dffe3..f664a03e 100644 --- a/pixl_imaging/src/pixl_imaging/main.py +++ b/pixl_imaging/src/pixl_imaging/main.py @@ -18,9 +18,9 @@ import asyncio import importlib.metadata -from core.telemetry import configure_logging from core.patient_queue.subscriber import PixlConsumer from core.rest_api.router import router, state +from core.telemetry import configure_logging from decouple import config from fastapi import FastAPI from fastapi.responses import JSONResponse From a3c6b8a444e29c9a9c9266e96d083ba3e021d9a1 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 11:48:18 +0100 Subject: [PATCH 14/15] Add comments to clarify why we need to reuse an existing otel provider --- pixl_core/src/core/logging.py | 3 +++ pixl_core/src/core/telemetry.py | 9 ++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/pixl_core/src/core/logging.py b/pixl_core/src/core/logging.py index 5727dca6..8dd2889f 100644 --- a/pixl_core/src/core/logging.py +++ b/pixl_core/src/core/logging.py @@ -63,6 +63,9 @@ def _build_provider(self) -> LoggerProvider: The provider is flushed on exit so we can include logs from short-lived processes, i.e. the CLI. """ + # If we have auto-instrumented the service, there's no way to tell the OTel SDK not to + # create the provider. So we have to reuse it here to avoid warnings in the logs. + # The provider created by the OTel SDK is equivalent to the one we create below. existing_provider = get_logger_provider() if isinstance(existing_provider, LoggerProvider): return existing_provider diff --git a/pixl_core/src/core/telemetry.py b/pixl_core/src/core/telemetry.py index 5951c14e..0ed7dfad 100644 --- a/pixl_core/src/core/telemetry.py +++ b/pixl_core/src/core/telemetry.py @@ -44,7 +44,7 @@ def telemetry_is_enabled() -> bool: """ disabled = config("OTEL_SDK_DISABLED", cast=bool) if disabled: - logger.debug("OTEL_SDK_DISABLED is set, skipping OTel configuration") + logger.warning("OTEL_SDK_DISABLED is set, skipping OTel configuration") return False endpoint = config("OTEL_EXPORTER_OTLP_ENDPOINT") @@ -85,6 +85,13 @@ def configure_tracing() -> None: if not telemetry_is_enabled(): return + # If we have auto-instrumented the service, there's no way to tell the OTel SDK not to + # create the provider. So we have to reuse it here to avoid warnings in the logs. + # The provider created by the OTel SDK is equivalent to the one we create below. + existing_provider = trace.get_tracer_provider() + if isinstance(existing_provider, TracerProvider): + return + exporter = OTLPSpanExporter() processor = BatchSpanProcessor(exporter) provider = TracerProvider(resource=Resource.create()) From 24052788aa9c6d4f6ca525cf5ef9dab94ce627b6 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 11:49:30 +0100 Subject: [PATCH 15/15] Make linters happy --- pixl_core/src/core/logging.py | 2 +- pixl_core/src/core/telemetry.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pixl_core/src/core/logging.py b/pixl_core/src/core/logging.py index 8dd2889f..3fc52f1f 100644 --- a/pixl_core/src/core/logging.py +++ b/pixl_core/src/core/logging.py @@ -65,7 +65,7 @@ def _build_provider(self) -> LoggerProvider: """ # If we have auto-instrumented the service, there's no way to tell the OTel SDK not to # create the provider. So we have to reuse it here to avoid warnings in the logs. - # The provider created by the OTel SDK is equivalent to the one we create below. + # The provider created by the OTel SDK is equivalent to the one we create below. existing_provider = get_logger_provider() if isinstance(existing_provider, LoggerProvider): return existing_provider diff --git a/pixl_core/src/core/telemetry.py b/pixl_core/src/core/telemetry.py index 0ed7dfad..17157e91 100644 --- a/pixl_core/src/core/telemetry.py +++ b/pixl_core/src/core/telemetry.py @@ -87,7 +87,7 @@ def configure_tracing() -> None: # If we have auto-instrumented the service, there's no way to tell the OTel SDK not to # create the provider. So we have to reuse it here to avoid warnings in the logs. - # The provider created by the OTel SDK is equivalent to the one we create below. + # The provider created by the OTel SDK is equivalent to the one we create below. existing_provider = trace.get_tracer_provider() if isinstance(existing_provider, TracerProvider): return