Commit 3805e4f
authored
observability: spec v0.7 OTel mapping (proposal 0007) — phase 6.0 (#19)
* observability: spec v0.7 OTel mapping (proposal 0007) — phase 6.0
Land the observability capability per spec proposal 0007. Adds
`openarmature.observability` with the §3 correlation_id ContextVar +
queue-mediated dispatch primitives (core, no OTel deps), and an
extras-gated `openarmature.observability.otel` subpackage with
`OTelObserver`, detached trace mode, LLM provider span hook, and the
log bridge.
Engine integration: `correlation_id` ContextVar set/reset around the
outermost `invoke()`; `_active_observers` and `_active_dispatch`
ContextVars set/reset around every chain invocation in
`_step_function_node` / `_step_subgraph_node` / `_step_fan_out_node`.
The LLM provider span hook in `OpenAIProvider.complete` calls
`current_dispatch()` to enqueue NodeEvent-shaped records on the same
delivery worker the engine uses, preserving spec §6 serial ordering.
OTelObserver implementation:
- Private TracerProvider per §6 — never registers globally.
- Subscribes to `started` / `completed` / `checkpoint_saved` phases.
- §4.5 subgraph dispatch span synthesis (the engine wrapper is
transparent per fixture 013, but observability mandates a span).
- §4.4 detached trace mode for subgraphs (one Link, two traces) and
fan-outs (one trace per instance, one Link per).
- §5 attribute population including the cross-cutting
`openarmature.correlation_id` on every span.
- §5.5 LLM provider span with `disable_llm_spans` opt-out.
- §10.8 checkpoint_saved → zero-duration
`openarmature.checkpoint.save` span.
- `spec_version` reads from `openarmature.__spec_version__` so the
three-place version sync covers the OTel surface automatically.
Charter alignment: OTel deps are optional via `pip install
openarmature[otel]`; correlation primitives stay in core. Plan to
lift `observability.otel` into a sibling `openarmature-otel` package
at the v1.0 launch alongside `openarmature-eval` per charter §3.2.
`opentelemetry-sdk>=1.27,<2` upper-bound guards against the SDK 2.x
LoggingHandler removal until the migration to
opentelemetry-instrumentation-logging lands.
Test coverage: 4 of 11 conformance fixtures driven end-to-end —
001 (basic trace), 005 (LLM provider span + isolation under
external auto-instrumentation), 008 (detached subgraph + detached
fan-out), 009 (correlation_id cross-cutting). The Phase 5 deferred
fixture 031 span/log assertions activate. The remaining 7 fixtures
defer to Phase 6.1 with per-fixture wiring notes (tracked in
openarmature-coord). 15 unit tests cover the engine path
independently of conformance harness wiring.
* observability: PR #19 review followups + CI extras fix
Three changes:
- **CI** — ``uv sync --frozen`` didn't install the ``[otel]`` extras,
so pyright type-checked ``tests/unit/test_observability_otel.py``
against missing ``opentelemetry.*`` symbols and produced 373
false-positive errors. Switched to ``--all-extras`` so every
optional-dependency group is present in CI.
- **OTelObserver.spec_version factory** — ``default_factory=lambda:
_read_spec_version()`` simplified to ``default_factory=_read_spec_version``;
the lambda was a redundant wrapper.
- **test_correlation.py** — replaced ``lambda s: _pre(s)`` with the
bare ``_pre``; the type-ignore came off too.
The other two unnecessary-lambda findings (``detached_subgraphs`` /
``detached_fan_outs`` factories) are kept with an explanatory comment
— pyright strict mode infers ``default_factory=frozenset`` as
``frozenset[Unknown]`` and flags it; the lambda preserves the
``frozenset[str]`` annotation.
* observability: PR #19 round-2 review followups
Five Copilot findings addressed:
- **`_close_subgraph_span` token leak** — was ending the synthetic
subgraph span without detaching the OTel context token, leaving
the current-span context pinned to a closed span. Added the
detach with the same cross-context guard as
``_close_detached_root``.
- **Detached fan-out instance parent lookup** — added an explicit
``namespace[:1] + (str(fan_out_index),)`` check at the top of
``_resolve_parent_context`` so inner-instance spans don't depend
on the attach-then-resolve ordering of the surrounding
``_sync_subgraph_spans`` call.
- **`isdigit()` heuristic** — replaced the string-shape guess for
identifying fan-out instance roots with an explicit
``_fan_out_instance_root_prefixes: set[tuple[str, ...]]`` so a
pure-digit node name (e.g. ``"123"``) doesn't get misclassified.
- **Invocation-span memory leak in long-lived observers** — closing
prior-correlation_id invocation spans when a new correlation_id's
first event arrives. Plus a public
``OTelObserver.close_invocation(correlation_id)`` method for
callers who want explicit control without driving a follow-on
invocation. The very-last invocation still closes on
``shutdown()``.
- **`current_dispatch` re-export** — added to
``openarmature.observability.__all__`` to match the public-API
intent in ``correlation.py``'s ``__all__`` and the PR description.
Plus one harness fix:
- **test_observability.py collection-time skip** — added
``pytest.importorskip("opentelemetry.trace")`` before the OTel
imports so the file skips cleanly when the ``otel`` extras
aren't installed (matches ``tests/unit/test_observability_otel.py``).
The two re-flagged ``frozenset`` lambdas are kept with the inline
comment explaining why pyright strict mode needs them — same
position as the prior PR review pass.
* observability: PR #19 round-3 review followups
Seven Copilot/code-quality findings addressed:
- **`_close_subgraph_span` empty-except comment** — added inline
rationale inside the `except ValueError` block (the explanation
was already above the `try`; rule wants it on the empty branch).
- **Concurrency limitation documented** — class docstring now spells
out that ``OTelObserver`` instances are NOT safe across concurrent
invocations (overlapping namespaces collide; close-prior-
correlation_id closes other in-flight invocations). Recommended
pattern: one observer per ``CompiledGraph`` for sequential
workloads, fresh per-invocation observers for parallel services.
Phase 6.1 tracks the proper correlation_id-scoped state fix.
- **Module docstring drift** — rewrote to reflect the
``(namespace, attempt_index, fan_out_index)`` leaf-span key (not
the prior ``(trace_id, ...)`` shape) plus the dedicated dicts
for subgraph dispatch, detached roots, and invocation spans.
- **Real ``invocation_id`` on the invocation span** — added
``current_invocation_id()`` ContextVar reader to ``correlation.py``
mirroring ``current_correlation_id``; engine sets it in
``invoke()`` before worker creation. ``OTelObserver`` reads it
when opening the invocation span and omits the attribute when
None (replaces the prior ``"<unset>"`` sentinel).
- **`_make_llm_event` phase typing** — tightened ``phase: str``
to ``Literal["started", "completed"]``; dropped the corresponding
``cast``.
- **Test-isolation for global TracerProvider** — saved + restored
the prior global provider in both
``test_observer_uses_private_provider_not_global`` and the
fixture 005 driver (the only sites that call
``set_tracer_provider``).
- **`test_correlation_id_and_invocation_id_are_structurally_distinct`** —
rewrote to drive a real invocation with a checkpointer and assert
``record.correlation_id != record.invocation_id`` deterministically;
also cross-checks via the new ``current_invocation_id()`` reader
inside the node body. The prior assertion was on stdlib
``uuid.uuid4()``, not framework behavior.
The Phase 6.1 conformance-fillin tracker (in openarmature-coord) is
updated to add the concurrency-safe state scoping work alongside
the deferred fixtures.
* observability: replace frozenset lambdas with named factory
Two github-code-quality findings on ``detached_subgraphs`` /
``detached_fan_outs`` defaults addressed via the bot's named-factory
suggestion. Previous rounds kept the lambdas with an explanatory
comment because ``default_factory=frozenset`` produced
``frozenset[Unknown]`` under pyright strict mode; the named factory
``_empty_str_frozenset() -> frozenset[str]`` carries the explicit
return annotation, satisfying both constraints (no lambda; pyright
preserves the typed empty frozenset). Removed the explanatory
comment along with the lambdas.
* codeql: suppress py/ineffectual-statement (false positives)
Adds ``.github/codeql/codeql-config.yml`` with a query-filter
exclude for ``py/ineffectual-statement``. The rule has produced
zero true positives across the codebase; every finding has been
the same false-positive pattern:
- PEP 544 ``Protocol`` method bodies (``...``) on the eight
Protocol classes the framework defines (``Provider``,
``Checkpointer``, ``Middleware``, ``Observer``, etc.). Canonical
Python idiom — never executed.
- ``await worker`` lines flagged as no-effect when ``await`` on
a task is the entire mechanism that waits for the task to
complete (CodeQL bug on async patterns).
The genuine "unused statement" cases are caught at type-check time
by pyright's ``reportUnusedExpression`` — suppressing the CodeQL
rule doesn't lose any signal.
To make GitHub's default-setup code scanning honor this config:
Settings → Code security → Code scanning → CodeQL default setup →
"Custom configuration" → point at
``.github/codeql/codeql-config.yml``. (Or switch to advanced
setup; the file works either way once selected.)
* codeql: switch to advanced setup with config file
Default setup's UI doesn't expose a config-file field, so the
``.github/codeql/codeql-config.yml`` we added to suppress
``py/ineffectual-statement`` had no effect. Switching to advanced
setup via an explicit workflow file fixes that — the workflow
references the config, and the rule exclusion lands.
Workflow matches default setup's coverage:
- Runs on ``main`` push, PRs to ``main``, plus a weekly scheduled
scan.
- Two language jobs (``actions``, ``python``).
- Includes the ``security-and-quality`` query suite explicitly so
code-quality findings still surface; only the per-rule exclusion
in the config file is silenced.
- Uses ``security-events: write`` permission per least-privilege.
The user disables default setup in Settings → Code security → CodeQL
analysis when this PR merges; the workflow takes over.
* observability: PR #19 round-5 review followups
Two Copilot findings addressed:
- **Log-bridge filter on the root logger** — ``install_log_bridge``
was attaching ``_CorrelationIdFilter`` to the OTel
``LoggingHandler`` only. Pre-existing stdout / file / third-party
handlers registered ahead of it on the root logger would see log
records WITHOUT ``openarmature.correlation_id``, violating spec
§7's "every log record emitted during an invocation MUST carry
the attribute." Moved the filter to ``root.addFilter(...)`` so
it runs at the LOGGER level, applying to every record regardless
of which handler processes it. Idempotency is now tracked
separately for handler vs filter installation.
- **Typed LLM event payload** — ``_make_llm_event`` previously
stuffed a plain dict into ``NodeEvent.pre_state`` with
``cast("Any", ...)``, violating the documented
``pre_state: State`` contract. Any observer calling
``event.pre_state.model_dump()`` on an LLM event would crash.
Wrapped the payload in a dedicated ``_LlmEventState(State)``
Pydantic subclass with the LLM fields (model, finish_reason,
prompt/completion/total tokens, optional error_type / message /
category). The OTel observer reads attributes directly via
``isinstance(event.pre_state, _LlmEventState)`` and attribute
access. Schema is honored.
* codeql: also suppress py/unused-import false positives
Same false-positive pattern as the prior ``py/ineffectual-statement``
exclusion — three cases CodeQL flags but isn't actually right about:
- Forward-reference casts (``cast("FinishReason", x)``,
``cast("Checkpointer", capturing)``). The string argument isn't
parsed by CodeQL but pyright resolves it; removing the import
yields ``reportUndefinedVariable``.
- Subscripted base classes (``class T(FanOutNode[State, State]):``)
— the base class IS used.
- Re-exports for public API.
Pyright's ``reportUnusedImport`` covers the genuine cases at
type-check time; the CodeQL rule duplicates with a worse FP rate.
* test_middleware: comment fix + nonlocal counter
Two Copilot suggestions on test_middleware.py:
- Comment said "more than twice" but the docstring + spec §2 say
"more than once" and the test pins at N=5 calls. Aligning the
comment.
- Replaced the ``inner_calls = [0]`` mutable-list closure trick
with ``nonlocal inner_calls`` + a plain int. Cleaner; same
behavior.
* test_llm_provider: docstring clarifications
Two Copilot suggestions on test_llm_provider.py — both pure
documentation:
- ``_make_provider`` — expanded to explain that the no-op
``MockTransport(lambda _req: httpx.Response(204))`` exists only
to satisfy provider construction; the tests using this helper
pass their own ``httpx.Response`` objects directly into
``_classify_http_error`` without going through the wire.
- ``test_complete_does_not_mutate_messages_or_tools`` — said the
input objects are "byte-identical" but the assertion is ``==``
(equality), not ``is`` (identity). Updated wording to "remain
equal to their pre-call snapshots."
* RELEASING.md: tighten version-string consistency
Three Copilot doc-quality findings:
- Commit message + PR title in §1 used the hyphenated
``0.5.0-rc1`` form, but ``pyproject.toml`` and ``__init__.py``
use the canonical PEP 440 ``0.5.0rc1`` form (no hyphen). Updated
both to ``0.5.0rc1`` so the literal strings copy-pasted from
the runbook match what the version files contain.
- Added a short paragraph at the top of §2 (Tag the RC) calling
out the two forms explicitly: version strings (pyproject /
``__init__.py``) use ``0.5.0rc1``; git tags use ``v0.5.0-rc1``.
PEP 440 normalizes them as equivalent (line 139 covers that),
but mixing them in commit messages and PR titles muddies the
audit trail.1 parent 689c336 commit 3805e4f
18 files changed
Lines changed: 2993 additions & 35 deletions
File tree
- .github
- codeql
- workflows
- docs
- src/openarmature
- graph
- llm/providers
- observability
- otel
- tests
- conformance
- unit
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
36 | 36 | | |
37 | 37 | | |
38 | 38 | | |
39 | | - | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
40 | 47 | | |
41 | 48 | | |
42 | 49 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
43 | 43 | | |
44 | 44 | | |
45 | 45 | | |
46 | | - | |
| 46 | + | |
47 | 47 | | |
48 | | - | |
| 48 | + | |
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
| |||
56 | 56 | | |
57 | 57 | | |
58 | 58 | | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
59 | 67 | | |
60 | 68 | | |
61 | 69 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
16 | 16 | | |
17 | 17 | | |
18 | 18 | | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
19 | 34 | | |
20 | 35 | | |
21 | 36 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
41 | 41 | | |
42 | 42 | | |
43 | 43 | | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
44 | 54 | | |
45 | 55 | | |
46 | 56 | | |
| |||
369 | 379 | | |
370 | 380 | | |
371 | 381 | | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
372 | 394 | | |
373 | 395 | | |
374 | 396 | | |
| |||
378 | 400 | | |
379 | 401 | | |
380 | 402 | | |
| 403 | + | |
| 404 | + | |
381 | 405 | | |
382 | 406 | | |
383 | 407 | | |
| |||
585 | 609 | | |
586 | 610 | | |
587 | 611 | | |
| 612 | + | |
| 613 | + | |
| 614 | + | |
| 615 | + | |
| 616 | + | |
| 617 | + | |
| 618 | + | |
| 619 | + | |
| 620 | + | |
588 | 621 | | |
589 | | - | |
590 | | - | |
591 | | - | |
592 | | - | |
593 | | - | |
594 | | - | |
595 | | - | |
| 622 | + | |
| 623 | + | |
| 624 | + | |
| 625 | + | |
| 626 | + | |
| 627 | + | |
| 628 | + | |
| 629 | + | |
| 630 | + | |
| 631 | + | |
| 632 | + | |
596 | 633 | | |
597 | 634 | | |
598 | 635 | | |
| |||
649 | 686 | | |
650 | 687 | | |
651 | 688 | | |
| 689 | + | |
| 690 | + | |
| 691 | + | |
| 692 | + | |
| 693 | + | |
| 694 | + | |
| 695 | + | |
652 | 696 | | |
653 | 697 | | |
654 | | - | |
655 | | - | |
656 | | - | |
657 | | - | |
658 | | - | |
659 | | - | |
660 | | - | |
661 | | - | |
662 | | - | |
663 | | - | |
| 698 | + | |
| 699 | + | |
| 700 | + | |
| 701 | + | |
| 702 | + | |
| 703 | + | |
| 704 | + | |
| 705 | + | |
| 706 | + | |
| 707 | + | |
| 708 | + | |
| 709 | + | |
| 710 | + | |
| 711 | + | |
664 | 712 | | |
665 | 713 | | |
666 | 714 | | |
| |||
740 | 788 | | |
741 | 789 | | |
742 | 790 | | |
| 791 | + | |
| 792 | + | |
| 793 | + | |
| 794 | + | |
| 795 | + | |
| 796 | + | |
| 797 | + | |
743 | 798 | | |
744 | | - | |
745 | | - | |
746 | | - | |
747 | | - | |
748 | | - | |
| 799 | + | |
| 800 | + | |
| 801 | + | |
| 802 | + | |
| 803 | + | |
| 804 | + | |
| 805 | + | |
| 806 | + | |
| 807 | + | |
749 | 808 | | |
750 | 809 | | |
751 | 810 | | |
| |||
0 commit comments