feat: add OpenTelemetry tracing (Phase B) - #69
Merged
Conversation
GuyZivRH
previously approved these changes
Aug 11, 2026
GuyZivRH
left a comment
Collaborator
There was a problem hiding this comment.
PR #69 Consolidated Review — OpenTelemetry tracing (Phase B)
PR: #69
Author: Aliciapet11 (apetruni)
Branch: feat/otel-integration → main
HEAD: 5e405c8
Size: +704 / −74 · 16 files · 2 commits
CI: SUCCESS (~50m, run 31389661392)
Ticket: APPENG-5370 Phase B
Merge state: BEHIND (slightly)
Consolidated: 2026-08-11 · prefix jzqwk_
Sources folded in: xqnlm_, lpmqx_, rptlw_, wkfnp_, xkntr_
Note: At consolidation time, Docs/commit_reviews/ only contained PR #69 reviews (earlier #57/#60/#64/#65 consolidations were no longer present).
Final verdict
Approve with minor fixes.
Consensus is Approve / Approve-with-fixes (4/5). One review (rptlw_) preferred Request changes for OTEL vs MLflow enablement inconsistency and AI co-author trailers — treat those as should-fix / clarify before treating dual mode as production, not as blockers for landing opt-in standalone OTLP on the scorecard path. CI green; design is sound.
What shipped
Lazy OpenTelemetry instrumentation (env-gated, optional deps, no-op fallback):
| Piece | Role |
|---|---|
abevalflow/observability/otel.py |
Lazy TracerProvider, shutdown, _NoOpTracer |
@timed_gate |
Gate spans: duration, pass/fail, score, mode; exception recording |
| 5 gates | cisco, skillmd-scanner, skillmd-quality, llm-review, edge-case |
llm_client.chat_completion_with_usage |
llm.chat_completion + token attrs |
aggregate_scorecard.main() |
Root span + setup/shutdown |
| Analyze Tekton task | otel-exporter-endpoint → OTEL_* env; pip install OTEL |
pyproject.toml [observability] |
OTEL API/SDK/OTLP gRPC |
| Tests | test_otel.py + LLM span tests (~17 new) |
| Claimed modes: standalone OTLP · MLflow-only · dual-export · neither = no-op. |
Consensus strengths
- Opt-in / zero default overhead; ImportError → no-op.
- Idempotent setup +
finallyshutdown for BatchSpanProcessor flush. - Useful attributes (gate outcome/score/mode; LLM model + tokens).
- Strong unit tests (env gating, dual env flag, span attrs, exceptions, no-op).
- CI green; author reports cluster no-op validation.
- Optional deps correctly under
observabilitynext to mlflow (#64).
Should fix / clarify (non-blocking for first merge)
| # | Issue | Ask |
|---|---|---|
| 1 | MLflow / dual mode honesty | Docstring says “activates only when OTEL endpoint set,” but is_otel_enabled() also keys off MLFLOW_TRACKING_URI. Dual/MLflow paths set env / return without set_tracer_provider — custom get_tracer() spans may silently no-op. Document standalone-OTLP-only for these spans or init a real export path + test. |
| 2 | Setup only in aggregate_scorecard.main() |
Gate/LLM export only inside that process after setup. Document Phase B scope or add setup at other entry points later. |
| 3 | Pipelines omit otel-exporter-endpoint |
Wire through CI/monitoring (empty default) or document enable steps. |
| 4 | Store installs OTEL pkgs unused | Remove or justify (no setup/spans on store path). |
| 5 | LLM span | Add record_exception / ERROR status on API failure (like timed_gate). |
| 6 | Failed gates | Set non-OK span status when passed=False; guard score is not None before set_attribute. |
| 7 | gRPC URL example | http://…:4317 may be wrong for gRPC exporter (often host:4317; HTTP is :4318). Document. |
| 8 | AI co-author trailers | Co-Authored-By: Claude / PR “Generated with…” — strip per repo convention. |
| 9 | Docs | Mark Phase B done in observability plan; note env vars. |
| 10 | Scope gaps (follow-up) | Engine gates not timed; gate_results.duration_ms not written to DB; prefer gen_ai.* / policy_key naming later (#57). |
Minority stance (rptlw_)
Would Request changes until (1) OTEL/MLflow enablement is consistent and not silently no-op while advertising modes, and (2) AI co-author trailers removed. Folded above as should-fix; does not overturn Approve if Phase B is explicitly “opt-in standalone OTLP on scorecard path.”
Related work
| PR | Interaction |
|---|---|
| #64 MLflow Observer | Dual-export env bridges Phase B ↔ C |
| #57 Join contract | Future: trace_id / run id as resource attrs |
| #56 Observability tables | duration_ms reserved, not populated yet |
Suggested GitHub action
- Approve (with comments on dual-mode + pipeline wiring).
- Ask for quick hygiene: rebase, drop AI co-author on future commits, clarify MLflow/dual in a comment or docs.
- Follow-up PR: pipeline param, store pip trim, LLM exception spans, failed-gate status.
Merge bar
- Env-gated no-op when unset
- Scorecard root span + shutdown
- Gate + LLM instrumentation + tests
- CI green
- (Follow-up) Dual/MLflow mode honesty or real provider init
- (Follow-up) Pipeline → analyze
otel-exporter-endpoint - Rebase on
main - (Hygiene) No AI co-author on future commits
Aliciapet11
added a commit
that referenced
this pull request
Aug 12, 2026
…ine wiring - Set ERROR span status on failed gates (passed=False) and guard score is not None before set_attribute - Add record_exception + ERROR status on LLM API failures - Wire otel-exporter-endpoint param through ci-pipeline and monitoring-pipeline YAMLs - Remove unused OTEL packages from store task pip install - Clarify gRPC endpoint format in task param description - Update otel.py docstrings for MLflow/dual mode honesty: Phase B spans only export in standalone OTLP mode - Add tests for failed-gate ERROR status, None score guard, and LLM exception span recording
GuyZivRH
approved these changes
Aug 12, 2026
- Add OTEL setup module with lazy-init TracerProvider, env-var gating, and graceful degradation when OTEL packages not installed - Enhance @timed_gate decorator to emit OTEL spans with gate name, duration, pass/fail, score, and mode attributes - Apply @timed_gate to all 5 gate evaluate methods (security, quality, behavioral) - Add OTEL span around LLM chat_completion_with_usage with model and token attributes - Add root span in aggregate_scorecard main() wrapping full scorecard aggregation with setup/shutdown lifecycle - Add OTEL packages to Tekton task pip installs and otel-exporter-endpoint param to analyze task - Add opentelemetry-api, opentelemetry-sdk, opentelemetry-exporter-otlp-proto-grpc to observability optional dependencies - Add 17 new tests for OTEL setup, timed_gate spans, and LLM client spans Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ine wiring - Set ERROR span status on failed gates (passed=False) and guard score is not None before set_attribute - Add record_exception + ERROR status on LLM API failures - Wire otel-exporter-endpoint param through ci-pipeline and monitoring-pipeline YAMLs - Remove unused OTEL packages from store task pip install - Clarify gRPC endpoint format in task param description - Update otel.py docstrings for MLflow/dual mode honesty: Phase B spans only export in standalone OTLP mode - Add tests for failed-gate ERROR status, None score guard, and LLM exception span recording
CI installs base deps only (no [observability] extras), so tests that import opentelemetry.sdk.trace fail with ModuleNotFoundError. Add skipif markers to TestTimedGateWithOtel and TestChatCompletionOtelSpan — tests run when SDK is available, skip gracefully when it's not.
Aliciapet11
force-pushed
the
feat/otel-integration
branch
from
August 12, 2026 12:41
8648500 to
c3508e3
Compare
Aliciapet11
added a commit
that referenced
this pull request
Aug 12, 2026
…ine wiring - Set ERROR span status on failed gates (passed=False) and guard score is not None before set_attribute - Add record_exception + ERROR status on LLM API failures - Wire otel-exporter-endpoint param through ci-pipeline and monitoring-pipeline YAMLs - Remove unused OTEL packages from store task pip install - Clarify gRPC endpoint format in task param description - Update otel.py docstrings for MLflow/dual mode honesty: Phase B spans only export in standalone OTLP mode - Add tests for failed-gate ERROR status, None score guard, and LLM exception span recording
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
abevalflow/observability/otel.py) with lazy-init TracerProvider, env-var gating, and graceful degradation when OTEL packages not installed@timed_gatedecorator to emit OTEL spans with gate name, duration, pass/fail, score, and mode attributes@timed_gateto all 5 gate evaluate methods (security, quality, behavioral)chat_completion_with_usagewith model and token attributesaggregate_scorecardmain()wrapping full scorecard aggregationotel-exporter-endpointparam to analyze taskopentelemetry-api,opentelemetry-sdk,opentelemetry-exporter-otlp-proto-grpcto observability optional dependenciesContext
Phase B of APPENG-5370. Tracing activates only when
OTEL_EXPORTER_OTLP_ENDPOINTis configured — zero overhead otherwise. Supports three modes: standalone OTEL (Jaeger/Tempo), MLflow-only tracing, or dual-export to both.Validation
ci-ase-otel-v2-96dncinalicia-evalflow) — all 6 tasks succeededTest plan
🤖 Generated with Claude Code