feat: add Pydantic Logfire tracing provider (#545) - #551
Conversation
amithad
left a comment
There was a problem hiding this comment.
Reviewed against the ak-dev skills (architecture, code quality, testing conventions, new-tracing-provider, write-spec, sync-docs-from-branch). CI is fully green.
Spec set (design.md / spec.md / plan.md / research/) — reviewed before the code. Every factual claim I checked verified against the base branch (trace factory lines, BaseTrace abstract methods, all five framework Module trace-selection sites, the Langfuse/OpenLLMetry reference lines, config/model/factory/test line numbers, pyproject extras). The staged format is followed well: point-form design, a spec that covers error handling, concurrency, and config compatibility, and a plan that orders the spec without restating it.
Implementation vs spec — every requirement extracted from design.md/spec.md is implemented as specified: package layout, once-guarded init(), five runners with the specified instrumentation split, factory branch + _BUILTIN_TRACERS, config description, logfire>=3.0 extra, example, tests, and the traceability docs. One gap is flagged inline (plan Iteration 5 skills sync). The spec's "instrumentors are internally idempotent" claim verifies: Logfire's LogfireTraceProviderWrapper.install returns early when already installed, and the OpenInference instrumentors carry BaseInstrumentor's guard.
Findings (no blockers):
- 2 x
[suggestion]inline: docs surfaces beyondtraceability.mdstill list only two providers; the streaming limitation is unstated in the spec and docs. - 1 x
[question]inline: plan Iteration 5's skills sync is not in the PR. [question]Staging: a brand-newdesign.mdships together withspec.md,plan.md, and the full implementation in one commit. The staged spec process expectsdesign.mdto survive review cycles before spec/implementation effort goes in. If the design was reviewed elsewhere (e.g. on issue #545), a note in the PR description would make that visible.[suggestion]examples/cli/logfireis not in the.github/test-config.yamle2e inventory. It runs with justOPENAI_API_KEY(token-less local mode), so it could join the matrix cheaply;examples/cli/smolagentsis also absent, so there is precedent either way.[nit]The branch's single commit message (feature: introduce pydantic logfire as a observarability tool) doesn't follow Conventional Commits (feat:) and has a typo ("observarability"); moot if squash-merged under the PR title.
Nothing was skipped as a duplicate: the PR had no prior review feedback.
Address PR #551 review feedback: - Update all doc surfaces that still listed only two tracing providers (root README, ak-py/README, configuration.md, overview.md, agent-skills.md) to include Logfire. - Update bundled user skills (ak-add-capabilities, ak-init, ak-build) so the shipped capability flow offers Logfire, and the dev skill ak-dev-new-tracing-provider so its _BUILTIN_TRACERS example and config description match the shipped list. - Document the streaming limitation: span wrapping applies to run() only (matching Langfuse/OpenLLMetry). Add a Non-goals bullet in design.md and qualify the traceability.md sentence. - Add examples/cli/logfire to the e2e test inventory (runs token-less with just OPENAI_API_KEY). - Note in plan.md Iteration 5 that the skills/docs sync is done in-PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review, @amithad. Addressed the findings in 78954f4: Doc surfaces beyond Streaming not wrapped (suggestion) — Confirmed against the runners (only Iteration 5 skills sync (question) — Done in-PR rather than deferred. e2e inventory (suggestion) — Added Staging / commit-message notes — Re the design-staging question: issue #545 has no design-review thread, so the staged |
…observability-tool # Conflicts: # ak-py/uv.lock
Description
Adds Pydantic Logfire as a third built-in observability/tracing provider alongside Langfuse and
OpenLLMetry, enabled with
trace.type: logfire. Logfire is an OpenTelemetry wrapper: the providerconfigures the global OTel tracer provider once at startup, and per-framework traced runners wrap each
agent run in a Logfire span carrying the
session_id. Deep instrumentation reuses the OpenInferenceinstrumentors already shipped by the framework extras (OpenAI uses Logfire's native
instrument_openai_agents()). Tracing stays transparent — enabling it is config-only, agent code isunchanged.
Ships the spec set (
design.md,spec.md,plan.md+ supporting research) underdocs/specs/545-logfire-tracing/.Type of Change
Related Issues
Fixes #545
Changes Made
ak-py/src/agentkernel/trace/logfire/:Logfire(BaseTrace)with a thread-safe,once-guarded
init()(logfire.configure(service_name="AgentKernel", send_to_logfire="if-token-present")),plus five traced runners (
openai,langgraph,crewai,adk,smolagents) that wrapsuper().run()inlogfire.span(...)and setinput/outputattributes.logfire.instrument_openai_agents(); CrewAI and Google ADKvia the OpenInference instrumentors already in those extras; LangGraph and Smolagents are span-level.
trace/trace.py: addedlogfireto_BUILTIN_TRACERSand arequire_extra-wrappedbuild branch (missing SDK → friendly
pip install "agentkernel[logfire]").core/config.py:_TraceConfig.typedescription now listslogfire(default unchanged).pyproject.toml: newlogfire = ["logfire>=3.0"]extra.examples/cli/logfire/: CLI OpenAI agent withtrace.type: logfire, README documentingLOGFIRE_TOKENand token-less local mode.tests/test_trace.py(logfire missing-extra) and newtests/test_trace_logfire.py(factory build, once-guard, span wrapping + attributes, error propagation, native instrumentation,
runner subclassing).
docs/docs/advanced/traceability.md(install, config, credentials,coverage table, troubleshooting) + the spec documents.
Testing
cd ak-py && uv run pytest→ 859 passed. New trace tests all pass. The only failures are 3pre-existing
test_cli_tester.pyLLM-judge tests that requireOPENAI_API_KEYin the environment(unrelated to this change). Smoke-tested against the real
logfire4.38 SDK:configure,span, andinstrument_openai_agentsverified, and all five runners construct/instrument end-to-end.make lint-check-allis clean.Checklist