From 241c6a7747c31cadd522fc5c2e2a52b05a31b2fd Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 12:06:31 +0100 Subject: [PATCH 01/16] Enable sending metrics to otlp endpoint --- docker-compose.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker-compose.yml b/docker-compose.yml index ded0743e..337787b7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -66,7 +66,7 @@ x-otel-common: &otel-common OTEL_EXPORTER_OTLP_PROTOCOL: grpc OTEL_LOGS_EXPORTER: none # we define our own loguru sink for exporting logs OTEL_TRACES_EXPORTER: otlp - OTEL_METRICS_EXPORTER: none + OTEL_METRICS_EXPORTER: otlp x-logs-volume: &logs-volume type: volume From 3c52c3f684633f6884602eaf4e0da77747d68f33 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 12:13:11 +0100 Subject: [PATCH 02/16] Add a core.metrics module to define custom metrics --- pixl_core/src/core/metrics.py | 60 +++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 pixl_core/src/core/metrics.py diff --git a/pixl_core/src/core/metrics.py b/pixl_core/src/core/metrics.py new file mode 100644 index 00000000..a4ec04a1 --- /dev/null +++ b/pixl_core/src/core/metrics.py @@ -0,0 +1,60 @@ +# 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. +"""Define custom metrics for PIXL.""" + +from __future__ import annotations + +from dataclasses import dataclass + +from opentelemetry import metrics + +__all__ = [ + "initialise_metrics", + "record_study_exported", +] + + +@dataclass +class PixlMetrics: + """Custom metrics for PIXL.""" + studies_exported: metrics.Counter | None = None + + +pixl_metrics = PixlMetrics() + + +def initialise_metrics() -> None: + """ + Initialise custom metrics for PIXL. + + This must be done after the metrics provider has been set up, + otherwise the metrics will be no-ops. + """ + meter = metrics.get_meter(__name__) + pixl_metrics.studies_exported = meter.create_counter( + name="pixl_studies_exported", + description="Number of studies exported, by project.", + unit="1", + ) + +def record_study_exported(project_name: str) -> None: + """ + Record a study exported metric. + + Args: + project_name (str): The name of the project for which the study was exported. + """ + if pixl_metrics.studies_exported is not None: + pixl_metrics.studies_exported.add(1, {"project_name": project_name}) + From e2b50f5d6e304181c34651bdb9ce1e0295a719a7 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 12:13:29 +0100 Subject: [PATCH 03/16] Add a core.telemetry.configure_metrics function --- pixl_core/src/core/telemetry.py | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/pixl_core/src/core/telemetry.py b/pixl_core/src/core/telemetry.py index 17157e91..ae3ef86c 100644 --- a/pixl_core/src/core/telemetry.py +++ b/pixl_core/src/core/telemetry.py @@ -21,16 +21,21 @@ from decouple import config from loguru import logger -from opentelemetry import trace +from opentelemetry import metrics, trace +from opentelemetry.exporter.otlp.proto.grpc.metric_exporter import OTLPMetricExporter from opentelemetry.exporter.otlp.proto.grpc.trace_exporter import OTLPSpanExporter +from opentelemetry.sdk.metrics import MeterProvider +from opentelemetry.sdk.metrics.export import PeriodicExportingMetricReader from opentelemetry.sdk.resources import Resource from opentelemetry.sdk.trace import TracerProvider from opentelemetry.sdk.trace.export import BatchSpanProcessor from core.logging import OTelSink +from core.metrics import initialise_metrics __all__ = [ "configure_logging", + "configure_metrics", "configure_tracing", "telemetry_is_enabled", ] @@ -98,3 +103,28 @@ def configure_tracing() -> None: provider.add_span_processor(processor) trace.set_tracer_provider(provider) atexit.register(provider.shutdown) + + +def configure_metrics() -> None: + """ + Set up an OTLP metric exporter when OTEL_SDK_DISABLED is false + and OTEL_EXPORTER_OTLP_ENDPOINT is set in the environment. + """ + 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 = metrics.get_meter_provider() + if isinstance(existing_provider, MeterProvider): + logger.debug("Existing MeterProvider detected (auto-instrumentation). Re-using it.") + initialise_metrics() + return + + exporter = OTLPMetricExporter() + reader = PeriodicExportingMetricReader(exporter) + provider = MeterProvider(resource=Resource.create(), metric_readers=[reader]) + metrics.set_meter_provider(provider) + atexit.register(provider.shutdown) + initialise_metrics() From a9a6ee5183a9bdfb047ca15cd5a53ef87f3f3145 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 12:14:28 +0100 Subject: [PATCH 04/16] call record_study_exported in the export api after a study is successfully exported --- pixl_export/src/pixl_export/main.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pixl_export/src/pixl_export/main.py b/pixl_export/src/pixl_export/main.py index fc7b8554..b7312ae4 100644 --- a/pixl_export/src/pixl_export/main.py +++ b/pixl_export/src/pixl_export/main.py @@ -23,6 +23,7 @@ from typing import Annotated from core.exports import ParquetExport +from core.metrics import configure_metrics, record_study_exported from core.rest_api.router import router from core.telemetry import configure_logging from core.uploader import get_uploader @@ -37,6 +38,8 @@ configure_logging(level=logging_level) logger.warning("Running logging at level {}", logging_level) +configure_metrics() + app = FastAPI( title="export-api", description="Export service", @@ -108,3 +111,4 @@ def export_dicom_from_orthanc( uploader = get_uploader(project_name) logger.debug("Sending {} via '{}'", study_id, type(uploader).__name__) uploader.upload_dicom_and_update_database(study_id) + record_study_exported(project_name) From 95efbcfdbfc37181c3669e27cf4eb243c8b528b6 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 12:14:36 +0100 Subject: [PATCH 05/16] Make linters happy --- pixl_core/src/core/metrics.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pixl_core/src/core/metrics.py b/pixl_core/src/core/metrics.py index a4ec04a1..56f139ff 100644 --- a/pixl_core/src/core/metrics.py +++ b/pixl_core/src/core/metrics.py @@ -28,6 +28,7 @@ @dataclass class PixlMetrics: """Custom metrics for PIXL.""" + studies_exported: metrics.Counter | None = None @@ -48,13 +49,14 @@ def initialise_metrics() -> None: unit="1", ) + def record_study_exported(project_name: str) -> None: """ Record a study exported metric. Args: project_name (str): The name of the project for which the study was exported. + """ if pixl_metrics.studies_exported is not None: pixl_metrics.studies_exported.add(1, {"project_name": project_name}) - From 88fe5fc363f110f77c9f515672c834e8526649f5 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 13:01:48 +0100 Subject: [PATCH 06/16] Add pixl_metrics.studies_exported metric, used in the orthanc-plugin --- orthanc/orthanc-anon/plugin/pixl.py | 15 ++++++++++-- pixl_core/src/core/metrics.py | 38 ++++++++++++++++++++++++++--- pixl_export/src/pixl_export/main.py | 4 +-- 3 files changed, 50 insertions(+), 7 deletions(-) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index 8d1c5b04..ce021733 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -35,8 +35,9 @@ import pydicom import requests from core.exceptions import PixlDiscardError, PixlSkipInstanceError +from core.metrics import record_study_deidentification_failure from core.project_config.pixl_config_model import load_project_config -from core.telemetry import configure_logging, configure_tracing +from core.telemetry import configure_logging, configure_metrics, configure_tracing from decouple import config from loguru import logger from opentelemetry import trace @@ -84,6 +85,8 @@ RequestsInstrumentor().instrument() tracer = trace.get_tracer("pixl.orthanc_anon") +configure_metrics() + logger.warning("Running logging at level {}", logging_level) # Set up a thread pool executor for non-blocking calls to Orthanc @@ -347,9 +350,17 @@ def _anonymise_study_and_upload( logger.warning( "Failed to anonymize project: '{}', {}: {}", project_name, study_info, discard ) + record_study_deidentification_failure( + project_name=project_name, + reason=type(discard).__name__, + ) return None - except Exception: # noqa: BLE001 + except Exception as e: # noqa: BLE001 logger.exception("Failed to anonymize project: '{}', {}", project_name, study_info) + record_study_deidentification_failure( + project_name=project_name, + reason=type(e).__name__, + ) return None with logger.contextualize(pseudo_study_uid=anonymised_study_uid): diff --git a/pixl_core/src/core/metrics.py b/pixl_core/src/core/metrics.py index 56f139ff..b9b10747 100644 --- a/pixl_core/src/core/metrics.py +++ b/pixl_core/src/core/metrics.py @@ -22,6 +22,7 @@ __all__ = [ "initialise_metrics", "record_study_exported", + "record_study_deidentification_failure", ] @@ -30,6 +31,7 @@ class PixlMetrics: """Custom metrics for PIXL.""" studies_exported: metrics.Counter | None = None + deidentification_failures: metrics.Counter | None = None pixl_metrics = PixlMetrics() @@ -43,12 +45,20 @@ def initialise_metrics() -> None: otherwise the metrics will be no-ops. """ meter = metrics.get_meter(__name__) + pixl_metrics.studies_exported = meter.create_counter( - name="pixl_studies_exported", + name="pixl.studies.exported", description="Number of studies exported, by project.", unit="1", ) + description = "Number of studies that failed to be de-identified, by project and failure reason." + pixl_metrics.deidentification_failures = meter.create_counter( + name="pixl.studies.deidentification.failures", + unit="1", + description=description, + ) + def record_study_exported(project_name: str) -> None: """ @@ -58,5 +68,27 @@ def record_study_exported(project_name: str) -> None: project_name (str): The name of the project for which the study was exported. """ - if pixl_metrics.studies_exported is not None: - pixl_metrics.studies_exported.add(1, {"project_name": project_name}) + if pixl_metrics.studies_exported is None: + return + + pixl_metrics.studies_exported.add( + amount=1, + attributes={"project_name": project_name}, + ) + + +def record_study_deidentification_failure(project_name: str, reason: str) -> None: + """ + Record a de-identification failure metric. + + Args: + reason (str): The reason for the de-identification failure. + project_name (str): The name of the project for which the de-identification failure occurred. + """ + if pixl_metrics.deidentification_failures is None: + return + + pixl_metrics.deidentification_failures.add( + amount=1, + attributes={"reason": reason, "project_name": project_name}, + ) diff --git a/pixl_export/src/pixl_export/main.py b/pixl_export/src/pixl_export/main.py index b7312ae4..26091321 100644 --- a/pixl_export/src/pixl_export/main.py +++ b/pixl_export/src/pixl_export/main.py @@ -23,9 +23,9 @@ from typing import Annotated from core.exports import ParquetExport -from core.metrics import configure_metrics, record_study_exported +from core.metrics import record_study_exported from core.rest_api.router import router -from core.telemetry import configure_logging +from core.telemetry import configure_logging, configure_metrics from core.uploader import get_uploader from decouple import config # type: ignore [import-untyped] from fastapi import Body, FastAPI, HTTPException From 162c5c3347182413fc34bd7f868460ff5f5686f1 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 13:02:09 +0100 Subject: [PATCH 07/16] Make linters happy --- pixl_core/src/core/metrics.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pixl_core/src/core/metrics.py b/pixl_core/src/core/metrics.py index b9b10747..751e4230 100644 --- a/pixl_core/src/core/metrics.py +++ b/pixl_core/src/core/metrics.py @@ -21,8 +21,8 @@ __all__ = [ "initialise_metrics", - "record_study_exported", "record_study_deidentification_failure", + "record_study_exported", ] @@ -52,7 +52,9 @@ def initialise_metrics() -> None: unit="1", ) - description = "Number of studies that failed to be de-identified, by project and failure reason." + description = ( + "Number of studies that failed to be de-identified, by project and failure reason." + ) pixl_metrics.deidentification_failures = meter.create_counter( name="pixl.studies.deidentification.failures", unit="1", @@ -70,7 +72,7 @@ def record_study_exported(project_name: str) -> None: """ if pixl_metrics.studies_exported is None: return - + pixl_metrics.studies_exported.add( amount=1, attributes={"project_name": project_name}, @@ -84,10 +86,11 @@ def record_study_deidentification_failure(project_name: str, reason: str) -> Non Args: reason (str): The reason for the de-identification failure. project_name (str): The name of the project for which the de-identification failure occurred. + """ if pixl_metrics.deidentification_failures is None: return - + pixl_metrics.deidentification_failures.add( amount=1, attributes={"reason": reason, "project_name": project_name}, From aa52315b98804a170954f39d3f28b36645a065df Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 13:02:48 +0100 Subject: [PATCH 08/16] Make linters happy --- pixl_core/src/core/metrics.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixl_core/src/core/metrics.py b/pixl_core/src/core/metrics.py index 751e4230..9dda37ea 100644 --- a/pixl_core/src/core/metrics.py +++ b/pixl_core/src/core/metrics.py @@ -85,7 +85,7 @@ def record_study_deidentification_failure(project_name: str, reason: str) -> Non Args: reason (str): The reason for the de-identification failure. - project_name (str): The name of the project for which the de-identification failure occurred. + project_name (str): Name of the project for which the de-identification failure occurred. """ if pixl_metrics.deidentification_failures is None: From 2e5975e3a475306071da63ce42dcd13052861893 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 14:04:11 +0100 Subject: [PATCH 09/16] Record the failure message and type separately Also except sqlalchemy DBAPIError messages separately --- orthanc/orthanc-anon/plugin/pixl.py | 20 ++++++++++++++++++-- pixl_core/src/core/metrics.py | 14 +++++++++----- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index ce021733..b6fb30ec 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -34,6 +34,7 @@ import pydicom import requests +from sqlalchemy.exc import DBAPIError from core.exceptions import PixlDiscardError, PixlSkipInstanceError from core.metrics import record_study_deidentification_failure from core.project_config.pixl_config_model import load_project_config @@ -352,14 +353,29 @@ def _anonymise_study_and_upload( ) record_study_deidentification_failure( project_name=project_name, - reason=type(discard).__name__, + failure_type="PixlDiscardError", + message="All instances have been skipped", + ) + return None + except DBAPIError as e: + logger.exception( + "Failed to anonymize project: '{}', {}: {}", project_name, study_info, e + ) + # Keep only the first line of the error message as otherwise the message contains + # the entire SQL query that failed. This would make the message have too high cardinality + # for the metric to be useful, and would make it hard to query for specific failure messages. + record_study_deidentification_failure( + project_name=project_name, + failure_type=type(e.orig).__name__, + message=str(e.orig).splitlines()[0], ) return None except Exception as e: # noqa: BLE001 logger.exception("Failed to anonymize project: '{}', {}", project_name, study_info) record_study_deidentification_failure( project_name=project_name, - reason=type(e).__name__, + failure_type=type(e).__name__, + message=str(e).splitlines()[0], ) return None diff --git a/pixl_core/src/core/metrics.py b/pixl_core/src/core/metrics.py index 9dda37ea..c3afcc7e 100644 --- a/pixl_core/src/core/metrics.py +++ b/pixl_core/src/core/metrics.py @@ -79,19 +79,23 @@ def record_study_exported(project_name: str) -> None: ) -def record_study_deidentification_failure(project_name: str, reason: str) -> None: +def record_study_deidentification_failure( + project_name: str, + failure_type: str, + message: str, +) -> None: """ Record a de-identification failure metric. Args: - reason (str): The reason for the de-identification failure. - project_name (str): Name of the project for which the de-identification failure occurred. - + project_name: The name of the project for which the de-identification failure occurred. + failure_type: The type of the failure. + message (str): The message for the de-identification failure. """ if pixl_metrics.deidentification_failures is None: return pixl_metrics.deidentification_failures.add( amount=1, - attributes={"reason": reason, "project_name": project_name}, + attributes={"project_name": project_name, "type": failure_type, "message": message}, ) From aba6130ee13738ac1ee78810c7ff6018e06517d5 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 14:12:56 +0100 Subject: [PATCH 10/16] Add docs on adding new custom metrics --- docs/setup/developer.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/setup/developer.md b/docs/setup/developer.md index 7917c71c..fdcc9125 100644 --- a/docs/setup/developer.md +++ b/docs/setup/developer.md @@ -169,3 +169,21 @@ When adding context: - Use the same field names across services so logs can be joined up - If a field has been pseudonymised, bind a new field prefixed with `pseudo_`, e.g. `study_uid` becomes `pseudo_study_uid` after pseudonymisation + +### Adding metrics + +Custom metrics are defined centrally in [`core.metrics`](../../pixl_core/src/core/metrics.py). +To add a new metric: + +1. Add a field for it on the `PixlMetrics` dataclass, and create the instrument + (e.g. a counter) in `initialise_metrics()`. Metric names use dots as + separators, e.g. `pixl.studies.exported`. +2. Add a `record_*` helper that records a value on the instrument. Guard against + the instrument being `None` (it is unset when telemetry is disabled) and + return early if so. +3. Call the `record_*` helper from the relevant service(s). + +You can pass `attributes` to the metric that can later be used for filtering and +aggregation, e.g. `project_name`. It's highly recommended to keep attribute values +**low-cardinality** - each distinct combination of attribute values creates a separate +time series, so avoid unbounded values like raw IDs or full tracebacks. From 8fbb6c8bd8f5045958c4f322065fcf8083059e7f Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 15:00:18 +0100 Subject: [PATCH 11/16] Add record_instance_deidentification_failure metric and use it in orthanc-anon --- orthanc/orthanc-anon/plugin/pixl.py | 23 ++++++++++++++++- pixl_core/src/core/metrics.py | 40 ++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 2 deletions(-) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index b6fb30ec..b25b23f3 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -36,7 +36,10 @@ import requests from sqlalchemy.exc import DBAPIError from core.exceptions import PixlDiscardError, PixlSkipInstanceError -from core.metrics import record_study_deidentification_failure +from core.metrics import ( + record_instance_deidentification_failure, + record_study_deidentification_failure, +) from core.project_config.pixl_config_model import load_project_config from core.telemetry import configure_logging, configure_metrics, configure_tracing from decouple import config @@ -439,6 +442,12 @@ def _anonymise_study_instances( ) key = "DICOM instance discarded as series not requested" skipped_instance_counts[key] += 1 + record_instance_deidentification_failure( + project_name=project_name, + study_uid=study_info.study_uid, + failure_type="PixlSkipSeriesError", + message=key, + ) continue if dataset.SeriesInstanceUID in series_to_skip: @@ -449,6 +458,12 @@ def _anonymise_study_instances( ) key = "DICOM instance discarded as series has too few instances" skipped_instance_counts[key] += 1 + record_instance_deidentification_failure( + project_name=project_name, + study_uid=study_info.study_uid, + failure_type="PixlSkipSeriesError", + message=key, + ) continue try: @@ -463,6 +478,12 @@ def _anonymise_study_instances( e, ) skipped_instance_counts[str(e)] += 1 + record_instance_deidentification_failure( + project_name=project_name, + study_uid=study_info.study_uid, + failure_type="PixlSkipInstanceError", + message=str(e), + ) else: anonymised_instances_bytes.append(anonymised_instance) anonymised_study_uid = dataset[0x0020, 0x000D].value diff --git a/pixl_core/src/core/metrics.py b/pixl_core/src/core/metrics.py index c3afcc7e..c50b6641 100644 --- a/pixl_core/src/core/metrics.py +++ b/pixl_core/src/core/metrics.py @@ -21,6 +21,7 @@ __all__ = [ "initialise_metrics", + "record_instance_deidentification_failure", "record_study_deidentification_failure", "record_study_exported", ] @@ -32,6 +33,7 @@ class PixlMetrics: studies_exported: metrics.Counter | None = None deidentification_failures: metrics.Counter | None = None + instance_deidentification_failures: metrics.Counter | None = None pixl_metrics = PixlMetrics() @@ -61,6 +63,14 @@ def initialise_metrics() -> None: description=description, ) + pixl_metrics.instance_deidentification_failures = meter.create_counter( + name="pixl.instances.deidentification.failures", + unit="1", + description=( + "Number of instances that failed to be de-identified, by project and failure reason." + ), + ) + def record_study_exported(project_name: str) -> None: """ @@ -85,7 +95,7 @@ def record_study_deidentification_failure( message: str, ) -> None: """ - Record a de-identification failure metric. + Record a study de-identification failure metric. Args: project_name: The name of the project for which the de-identification failure occurred. @@ -99,3 +109,31 @@ def record_study_deidentification_failure( amount=1, attributes={"project_name": project_name, "type": failure_type, "message": message}, ) + + +def record_instance_deidentification_failure( + project_name: str, + study_uid: str, + failure_type: str, + message: str, +) -> None: + """ + Record an instance de-identification failure metric. + + Args: + project_name: The name of the project for which the de-identification failure occurred. + study_uid: The UID of the study for which the de-identification failure occurred. + failure_type: The type of the failure. + message (str): The message for the de-identification failure. + """ + if pixl_metrics.instance_deidentification_failures is None: + return + + pixl_metrics.instance_deidentification_failures.add( + amount=1, + attributes={ + "project_name": project_name, + "study_uid": study_uid, + "type": failure_type, + "message": message}, + ) From c880965278ce7fc4daf93ed7614ae7e338c5dd3e Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 16:01:19 +0100 Subject: [PATCH 12/16] Add metrics from rabbitmq Scrape the metrics from the prometheus endpoint rabbitmq exposes --- .env.sample | 2 ++ docker-compose.yml | 1 + docs/setup/developer.md | 14 +++++++++++ test/docker-compose.yml | 2 ++ test/prometheus.yaml | 53 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 72 insertions(+) create mode 100644 test/prometheus.yaml diff --git a/.env.sample b/.env.sample index 20ec9860..75b02a9e 100644 --- a/.env.sample +++ b/.env.sample @@ -122,3 +122,5 @@ ENV= OTEL_SDK_DISABLED=true # gRPC endpoint of the OTel collector, e.g. http://localhost:4317 OTEL_EXPORTER_OTLP_ENDPOINT= +# Port for the PIXL RabbitMQ metrics endpoint, which is scraped by Prometheus to collect queue backlog depth. +RABBITMQ_METRICS_PORT=15692 diff --git a/docker-compose.yml b/docker-compose.yml index 337787b7..a78df44f 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -265,6 +265,7 @@ services: ports: - "127.0.0.1:${RABBITMQ_PORT}:5672" - "${RABBITMQ_ADMIN_PORT}:15672" + - "${RABBITMQ_METRICS_PORT}:15692" networks: - pixl-net volumes: diff --git a/docs/setup/developer.md b/docs/setup/developer.md index fdcc9125..6981915b 100644 --- a/docs/setup/developer.md +++ b/docs/setup/developer.md @@ -187,3 +187,17 @@ You can pass `attributes` to the metric that can later be used for filtering and aggregation, e.g. `project_name`. It's highly recommended to keep attribute values **low-cardinality** - each distinct combination of attribute values creates a separate time series, so avoid unbounded values like raw IDs or full tracebacks. + +### RabbitMQ queue metrics + +The RabbitMQ Docker image includes a Prometheus endpoint that exposes queue backlog depth metrics. +We use this to scrape queue depth metrics for each queue in the PIXL RabbitMQ broker, rather +than manually defining a metric within PIXL. This does, however, require defining a scrape job in +the [Prometheus configuration](../../test/prometheus.yaml) of the OTel Collector, +although the configuration is fairly minimal. + +Because metrics are scraped from the Prometheus endpoint, they are independent of +`OTEL_SDK_DISABLED`. This means queue metrics will always be collected whenever +something is scraping the endpoint. + +Note, queue depth is per-queue only, and cannot be broken down by project. diff --git a/test/docker-compose.yml b/test/docker-compose.yml index 38dbff42..26dd454d 100644 --- a/test/docker-compose.yml +++ b/test/docker-compose.yml @@ -27,6 +27,8 @@ services: ports: - "127.0.0.1:3000:3000" # Grafana UI - "127.0.0.1:4317:4317" # OTLP gRPC + volumes: + - ./prometheus.yaml:/otel-lgtm/prometheus.yaml:ro networks: pixl-net: diff --git a/test/prometheus.yaml b/test/prometheus.yaml new file mode 100644 index 00000000..c11203f7 --- /dev/null +++ b/test/prometheus.yaml @@ -0,0 +1,53 @@ +--- +# Based on https://github.com/grafana/docker-otel-lgtm/blob/main/docker/prometheus.yaml +# Adds the PIXL RabbitMQ metrics endpoint to the scrape configs. +global: + scrape_native_histograms: true +otlp: + keep_identifying_resource_attributes: true + # Recommended attributes to be promoted to labels. + promote_resource_attributes: + - service.instance.id + - service.name + - service.namespace + - service.version + - cloud.availability_zone + - cloud.region + - container.name + - deployment.environment # backward compatibility + - deployment.environment.name + - k8s.cluster.name + - k8s.container.name + - k8s.cronjob.name + - k8s.daemonset.name + - k8s.deployment.name + - k8s.job.name + - k8s.namespace.name + - k8s.node.name + - k8s.pod.name + - k8s.replicaset.name + - k8s.statefulset.name + - host.name + - postgresql.database.name + - postgresql.schema.name + - postgresql.table.name + - postgresql.index.name + - database # used by otelcol/receiver/mongodb + - kafka.cluster.alias +storage: + tsdb: + # A 10min time window is enough because it can easily absorb retries and network delays. + out_of_order_time_window: 10m +scrape_configs: + - job_name: rabbitmq + metrics_path: /metrics/detailed + params: + family: + - queue_coarse_metrics + - queue_consumer_count + static_configs: + - targets: + - queue:15692 + labels: + service_name: queue + service_namespace: pixl From 00e362fb759d9192135c5e344075f30d4b1f3c8a Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 16:55:42 +0100 Subject: [PATCH 13/16] Add unit tests for custom metrics --- pixl_core/tests/test_metrics.py | 95 +++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 pixl_core/tests/test_metrics.py diff --git a/pixl_core/tests/test_metrics.py b/pixl_core/tests/test_metrics.py new file mode 100644 index 00000000..1ffd75b4 --- /dev/null +++ b/pixl_core/tests/test_metrics.py @@ -0,0 +1,95 @@ +# 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 PIXL custom OpenTelemetry metrics.""" + +from __future__ import annotations + +from unittest.mock import Mock + +import pytest + +from core.metrics import ( + pixl_metrics, + record_instance_deidentification_failure, + record_study_deidentification_failure, + record_study_exported, +) + + +@pytest.fixture +def mock_instruments(monkeypatch: pytest.MonkeyPatch) -> dict[str, Mock]: + """ + Replace each instrument on pixl_metrics with a Mock counter. + + Lets the record_* helpers be tested by asserting on the counter's `.add` + calls, with no OTel provider or exporter involved. + """ + mocks = { + "studies_exported": Mock(), + "deidentification_failures": Mock(), + "instance_deidentification_failures": Mock(), + } + for name, mock in mocks.items(): + monkeypatch.setattr(pixl_metrics, name, mock) + return mocks + + +def test_record_study_exported(mock_instruments: dict[str, Mock]) -> None: + """Test the exported-studies counter is incremented with the project attribute.""" + record_study_exported(project_name="test-project") + + mock_instruments["studies_exported"].add.assert_called_once_with( + amount=1, + attributes={"project_name": "test-project"}, + ) + + +def test_record_study_deidentification_failure(mock_instruments: dict[str, Mock]) -> None: + """Test the study de-id failure counter records project, type and message attributes.""" + record_study_deidentification_failure( + project_name="test-project", + failure_type="StringDataRightTruncation", + message="value too long for type character varying(255)", + ) + + mock_instruments["deidentification_failures"].add.assert_called_once_with( + amount=1, + attributes={ + "project_name": "test-project", + "type": "StringDataRightTruncation", + "message": "value too long for type character varying(255)", + }, + ) + + +def test_record_instance_deidentification_failure(mock_instruments: dict[str, Mock]) -> None: + """Test the instance de-id failure counter records the study_uid alongside the rest.""" + record_instance_deidentification_failure( + project_name="test-project", + study_uid="1.2.3", + failure_type="ValueError", + message="bad tag", + ) + + mock_instruments["instance_deidentification_failures"].add.assert_called_once_with( + amount=1, + attributes={ + "project_name": "test-project", + "study_uid": "1.2.3", + "type": "ValueError", + "message": "bad tag", + }, + ) + + From 93733a568a59676596caafac04c01c20d13c6db0 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 16:58:52 +0100 Subject: [PATCH 14/16] Make linters happy --- orthanc/orthanc-anon/plugin/pixl.py | 7 ++++--- pixl_core/src/core/metrics.py | 5 ++++- pixl_core/tests/test_metrics.py | 2 -- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index b25b23f3..9050fa56 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -34,7 +34,6 @@ import pydicom import requests -from sqlalchemy.exc import DBAPIError from core.exceptions import PixlDiscardError, PixlSkipInstanceError from core.metrics import ( record_instance_deidentification_failure, @@ -57,6 +56,7 @@ write_dataset_to_bytes, ) from pydicom import dcmread +from sqlalchemy.exc import DBAPIError import orthanc @@ -365,8 +365,9 @@ def _anonymise_study_and_upload( "Failed to anonymize project: '{}', {}: {}", project_name, study_info, e ) # Keep only the first line of the error message as otherwise the message contains - # the entire SQL query that failed. This would make the message have too high cardinality - # for the metric to be useful, and would make it hard to query for specific failure messages. + # the entire SQL query that failed. This would make the message have too high + # cardinality for the metric to be useful, and would make it hard to query for + # specific failure messages. record_study_deidentification_failure( project_name=project_name, failure_type=type(e.orig).__name__, diff --git a/pixl_core/src/core/metrics.py b/pixl_core/src/core/metrics.py index c50b6641..e610e14b 100644 --- a/pixl_core/src/core/metrics.py +++ b/pixl_core/src/core/metrics.py @@ -101,6 +101,7 @@ def record_study_deidentification_failure( project_name: The name of the project for which the de-identification failure occurred. failure_type: The type of the failure. message (str): The message for the de-identification failure. + """ if pixl_metrics.deidentification_failures is None: return @@ -125,6 +126,7 @@ def record_instance_deidentification_failure( study_uid: The UID of the study for which the de-identification failure occurred. failure_type: The type of the failure. message (str): The message for the de-identification failure. + """ if pixl_metrics.instance_deidentification_failures is None: return @@ -135,5 +137,6 @@ def record_instance_deidentification_failure( "project_name": project_name, "study_uid": study_uid, "type": failure_type, - "message": message}, + "message": message, + }, ) diff --git a/pixl_core/tests/test_metrics.py b/pixl_core/tests/test_metrics.py index 1ffd75b4..bb280a02 100644 --- a/pixl_core/tests/test_metrics.py +++ b/pixl_core/tests/test_metrics.py @@ -91,5 +91,3 @@ def test_record_instance_deidentification_failure(mock_instruments: dict[str, Mo "message": "bad tag", }, ) - - From e7106ce15b56f6820174a469efda191707cdff47 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 17:21:37 +0100 Subject: [PATCH 15/16] Define RABBITMQ_METRICS_PORT for the system tests --- test/.env | 1 + 1 file changed, 1 insertion(+) diff --git a/test/.env b/test/.env index 0210b332..aa38e552 100644 --- a/test/.env +++ b/test/.env @@ -8,6 +8,7 @@ PIXL_MAX_MESSAGES_IN_FLIGHT=5 TZ=Europe/London OTEL_SDK_DISABLED=false OTEL_EXPORTER_OTLP_ENDPOINT=http://lgtm:4317 +RABBITMQ_METRICS_PORT=15692 # PIXL PostgreSQL instance PIXL_DB_HOST=postgres From a9f74a51ecab1da3b6b745957fb36eb82fb60ba1 Mon Sep 17 00:00:00 2001 From: Paul Smith Date: Wed, 1 Jul 2026 17:22:20 +0100 Subject: [PATCH 16/16] Make linters happy --- test/prometheus.yaml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/prometheus.yaml b/test/prometheus.yaml index c11203f7..db99219d 100644 --- a/test/prometheus.yaml +++ b/test/prometheus.yaml @@ -1,3 +1,16 @@ +# 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. --- # Based on https://github.com/grafana/docker-otel-lgtm/blob/main/docker/prometheus.yaml # Adds the PIXL RabbitMQ metrics endpoint to the scrape configs.