Skip to content

observability: phase 6.1 PR-B — logs SDK migration + filter bug fix#23

Merged
chris-colinsky merged 1 commit into
mainfrom
chore/phase-6-1-pr-b-logs-sdk-migration
May 8, 2026
Merged

observability: phase 6.1 PR-B — logs SDK migration + filter bug fix#23
chris-colinsky merged 1 commit into
mainfrom
chore/phase-6-1-pr-b-logs-sdk-migration

Conversation

@chris-colinsky
Copy link
Copy Markdown
Member

Summary

PR-B of Phase 6.1: migrate install_log_bridge off the
deprecated opentelemetry.sdk._logs.LoggingHandler (which OTel
SDK 2.x will remove) onto opentelemetry-instrumentation-logging
plus fix a real spec §7 violation surfaced during impl.

The migration (the agreed scope)

  • Handler import:
    opentelemetry.sdk._logs.LoggingHandler
    opentelemetry.instrumentation.logging.handler.LoggingHandler.
    Constructor call shape is identical
    (LoggingHandler(level=…, logger_provider=…)).
  • [otel] extras gain
    opentelemetry-instrumentation-logging>=0.62.0b1 (no upper
    bound — the contrib repo cycles fast on minor releases below
    1.0; revisit when 1.0 lands).
  • opentelemetry-sdk upper bound bumped from <2 to <3;
    same shape on opentelemetry-api for parity. We no longer
    reach into the deprecated path that prompted the original cap.
  • Phase 6.0 pending-migration comment block removed from
    pyproject.toml.
  • DeprecationWarning emitted by the prior implementation is gone
    (suite warnings count: 4 → 3; the 3 remaining are pre-existing
    intentional UserWarnings from fixture
    015-observer-error-isolation).

The filter bug (in-scope after impl review)

PR-B's new export-path test surfaced a real spec §7 violation
that's been latent since Phase 6.0: _CorrelationIdFilter was
attached to the root logger, but Python's logging propagation
walks ancestor handlers and skips ancestor filters. So records
emitted via child loggers — i.e., the normal
logging.getLogger("module") pattern — shipped to the OTel
exporter without the §7-mandated openarmature.correlation_id
attribute.

The existing test_log_bridge_filter_injects_correlation_id
only called flt.filter() directly, never integration-tested
through install_log_bridge + a child-logger emit + an export,
which is why the bug went unobserved through Phase 6.0 review,
round-7 followups, PR-A merge, and the fixture-031 span tests.

Fix: replace _CorrelationIdFilter with a process-global
logging.setLogRecordFactory hook. The factory chains over any
prior factory (preserving user-installed factories), reads
current_correlation_id() at record construction, and sets the
attribute on every newly constructed record — but only when a
correlation_id is in scope per §7's "within an invocation"
qualifier. _FACTORY_MARKER attribute on the wrapper function
makes re-installs no-ops; no stacked-wrapper pathology.

In-scope decision documented in
openarmature-coord/threads/phase-6-1-pr-b-logs-sdk-migration/
(03-python + 04-spec): the alternative was a separate fix-PR
that would have shipped a deprecation-free but still spec-§7-broken
bridge, with the new export-path test either papering over the
bug or being deferred. Spec agent confirmed Path 1 (fix in PR-B)
is the right call.

Tests

Three changes in tests/unit/test_observability_otel.py:

  • Renamed test_log_bridge_filter_injects_correlation_id
    test_log_record_factory_injects_correlation_id. Calls the
    installed factory directly via logging.getLogRecordFactory();
    asserts both null-cid (no attribute) and live-cid (attribute
    populated) paths. Restores the prior factory in finally.
  • Extended test_install_log_bridge_is_idempotent to also
    assert factory identity is preserved across re-calls (no second
    wrapper stacked on top of the first). Wrapped in
    warnings.catch_warnings("error").
  • Added test_log_bridge_exports_records_with_correlation_id
    — emits on a CHILD logger (the load-bearing case the prior
    implementation got wrong) and asserts the exported OTel
    LogRecord.attributes carry
    openarmature.correlation_id. Wrapped in
    warnings.catch_warnings("error").

Test plan

  • uv run pytest -q — 388 passed, 9 skipped (was 387 + 9;
    net +1 from the new export-path test)
  • uv run pyright — 0 errors
  • uv run ruff check . && uv run ruff format — clean
  • DeprecationWarning gone; 3 warnings remain (all pre-existing
    intentional UserWarnings)
  • Existing 35 unit tests + 5 conformance fixtures preserved

The OTel SDK's ``opentelemetry.sdk._logs.LoggingHandler`` is
deprecated and slated for removal in SDK 2.x. Migrate
``install_log_bridge`` to use the handler from
``opentelemetry-instrumentation-logging`` (the package the SDK's
deprecation message points at) so the deprecation goes away and
the bridge keeps working past SDK 2.x.

Constructor shape is unchanged (``LoggingHandler(level=...,
logger_provider=...)``); the contrib package's main public class
is ``LoggingInstrumentor`` but it's too opinionated for our use
(reads ``get_logger_provider()`` globally and overrides the
logging format) — direct handler import is the documented escape
hatch for callers like us with a caller-supplied LoggerProvider.

Dependency changes:

- Add ``opentelemetry-instrumentation-logging>=0.62.0b1`` to the
  ``[otel]`` extras. No upper bound — the contrib repo cycles
  fast on minor releases below 1.0; revisit when 1.0 lands.
- Drop ``<2`` upper bound on ``opentelemetry-sdk``; replace with
  ``<3`` as a coarse safety cap. We no longer reach into the
  deprecated path that prompted the original cap. Same shape on
  ``opentelemetry-api`` for parity.
- Remove the Phase 6.0 pending-migration comment block from
  ``pyproject.toml``.

The migration's new export-path test surfaced a real spec §7
violation in the existing bridge: ``_CorrelationIdFilter`` was
attached to the root logger, but Python's logging propagation
walks ancestor handlers and SKIPS ancestor filters, so child-
logger records (the normal ``logging.getLogger("module")``
pattern) shipped to the OTel exporter without the §7-mandated
``openarmature.correlation_id`` attribute. The existing
``test_log_bridge_filter_injects_correlation_id`` only called
``flt.filter()`` directly, never integration-tested through
``install_log_bridge`` + a child-logger emit + an export, which
is why the bug went unobserved through Phase 6.0 review,
round-7 followups, PR-A merge, and the fixture-031 span tests.

Fix: replace ``_CorrelationIdFilter`` with a process-global
``logging.setLogRecordFactory`` hook. The factory chains over any
prior factory (preserving user-installed factories), reads
``current_correlation_id()`` at record construction, and sets
``openarmature.correlation_id`` on every newly constructed record
— but only when a correlation_id is in scope per §7's "within
an invocation" qualifier. ``_FACTORY_MARKER`` attribute on the
wrapper function turns re-calls into no-ops; no stacked-wrapper
pathology.

Tests:

- ``test_log_bridge_filter_injects_correlation_id`` →
  ``test_log_record_factory_injects_correlation_id`` (calls the
  installed factory directly, asserts null-cid + live-cid paths).
- ``test_install_log_bridge_is_idempotent`` extended to assert
  factory identity is preserved across re-calls. Wrapped in
  ``warnings.catch_warnings("error")`` so the migrated path's
  "no DeprecationWarning" guarantee is locked in.
- New ``test_log_bridge_exports_records_with_correlation_id``
  emits on a CHILD logger (the load-bearing case) and asserts
  the exported OTel ``LogRecord.attributes`` carry
  ``openarmature.correlation_id``. Wrapped in
  ``warnings.catch_warnings("error")``.

388 tests pass (was 387; net +1 from the new export-path test).
Pyright clean. The DeprecationWarning emitted by the prior
implementation is gone (warnings count: 4 → 3, the 3 remaining
are pre-existing intentional ``UserWarning``s from fixture
015-observer-error-isolation).
Copilot AI review requested due to automatic review settings May 8, 2026 15:43
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the OpenTelemetry log-bridge integration to (1) migrate off the deprecated opentelemetry.sdk._logs.LoggingHandler to the opentelemetry-instrumentation-logging handler, and (2) fix a spec §7 correlation-id injection bug by moving from a root-logger filter to a process-global logging.setLogRecordFactory hook so child logger emissions also receive openarmature.correlation_id.

Changes:

  • Replace use of the deprecated OTel SDK LoggingHandler with opentelemetry.instrumentation.logging.handler.LoggingHandler and update [otel] extras / lockfile accordingly.
  • Replace the root-logger _CorrelationIdFilter approach with an idempotent LogRecord factory wrapper to ensure correlation IDs are attached for child logger emits.
  • Expand/adjust unit tests to cover factory idempotency and an end-to-end export path asserting openarmature.correlation_id reaches exported OTel LogRecord.attributes.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.

File Description
uv.lock Adds opentelemetry-instrumentation-logging (and transitive deps) to support the new handler import path.
pyproject.toml Updates [otel] extras to include instrumentation-logging and bumps opentelemetry-{api,sdk} upper bounds to <3.
src/openarmature/observability/otel/logs.py Migrates handler import and replaces filter-based correlation injection with an idempotent LogRecord factory wrapper.
tests/unit/test_observability_otel.py Updates/extends tests to validate factory behavior, idempotency, and correlation-id presence on exported log records from a child logger.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@chris-colinsky chris-colinsky merged commit 5afd325 into main May 8, 2026
9 checks passed
@chris-colinsky chris-colinsky deleted the chore/phase-6-1-pr-b-logs-sdk-migration branch May 8, 2026 15:53
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.

2 participants