Skip to content

Manually instrument traces in pixl_cli, orthanc_raw, and orthanc_anon#644

Open
p-j-smith wants to merge 10 commits into
mainfrom
paul/638-manually-instrument-traces
Open

Manually instrument traces in pixl_cli, orthanc_raw, and orthanc_anon#644
p-j-smith wants to merge 10 commits into
mainfrom
paul/638-manually-instrument-traces

Conversation

@p-j-smith

@p-j-smith p-j-smith commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Description

Fixes #639 Instrument the cli, Orthanc Raw, and Orthanc Anon so we can follow a DICOM study end-to-end

  • add a new function pixl_core.tracing.configure_tracing to set up a tracing provider
  • instrument the CLI. We first need to set relevant environment variables, which is done using _configure_telemetry_env_vars. Then configure tracing and instrument pika
  • manually start a span before each message to the queue
  • instrument Orthanc Raw. This is currently only instrumented for record_dicom_headers, not for the actual C-MOVE
  • instrument Orthanc Anon
  • add tests to check logs and traces are correlated
  • move the fixtures created for testing logging into the conftest so they can be reused for testing tracing

Type of change

Please delete options accordingly to the description.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Suggested Checklist

  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have passed on my local host device. (see further details at the CONTRIBUTING document)
  • Make sure your branch is up-to-date with main branch. See CONTRIBUTING for a general example to syncronise your branch with the main branch.
  • I have requested review to this PR.
  • I have addressed and marked as resolved all the review comments in my PR.
  • Finally, I have selected squash and merge

All logs selected for a given trace_id (the trace corresponds to processing one of the DICOM studies in the integration tests):

logs.mov

All spans for the same trace_id

trace.mov

@p-j-smith p-j-smith requested a review from stefpiatek June 18, 2026 13:13
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.83%. Comparing base (df58691) to head (bea0c56).

Files with missing lines Patch % Lines
cli/src/pixl_cli/main.py 91.66% 1 Missing ⚠️
pixl_core/src/core/patient_queue/producer.py 92.85% 1 Missing ⚠️
pixl_core/src/core/tracing.py 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #644      +/-   ##
==========================================
+ Coverage   87.79%   87.83%   +0.04%     
==========================================
  Files          78       79       +1     
  Lines        3735     3772      +37     
==========================================
+ Hits         3279     3313      +34     
- Misses        456      459       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stefpiatek stefpiatek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import BatchSpanProcessor

__all__ = ["configure_tracing"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, what are we aiming to exclude exporting here?

@pytest.fixture
def otel_logger(
monkeypatch: pytest.MonkeyPatch,
log_exporter: InMemoryLogRecordExporter,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fun

Comment thread docker-compose.yml

x-otel-common: &otel-common
OTEL_EXPORTER_OTLP_ENDPOINT: ${OTEL_EXPORTER_OTLP_ENDPOINT:-}
OTEL_EXPORTER_OTLP_HEADERS: ${OTEL_EXPORTER_OTLP_HEADERS:-}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh do we never use this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add manual OTel instrumentation where needed

2 participants