OLS-3536 Replace lifecycle span model with per-phase independent traces#336
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAudit behavior is renamed from logging to tracing. Runtime setup always exports spans to stdout and optionally OTLP. AgenticRun auditing now uses independent phase spans with links, span events, idempotent approval and terminal emissions, cleanup handling, and automatic-approval suppression. ChangesAudit tracing
Sequence Diagram(s)sequenceDiagram
participant AgenticRunReconciler
participant ProductionAuditLogger
participant OpenTelemetry
AgenticRunReconciler->>ProductionAuditLogger: Start phase span
ProductionAuditLogger->>OpenTelemetry: create phase span and link prior phase
AgenticRunReconciler->>ProductionAuditLogger: emit phase event
ProductionAuditLogger->>OpenTelemetry: record event attributes
AgenticRunReconciler->>ProductionAuditLogger: emit terminal span and cleanup
ProductionAuditLogger->>OpenTelemetry: end terminal span and clear state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
85a4310 to
a07c252
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
controller/agenticrun/audit_test.go (1)
770-787: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the auto-approval branch instead of manually omitting the call.
This test passes solely because it never calls
EmitApprovalSpan; it does not exerciseisAutoApprovedByPolicyorhandleExecution. Drive the handler with an automatic policy and assert the recorder contains no approval span.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/agenticrun/audit_test.go` around lines 770 - 787, The test TestNoApprovalSpan_AutoApproveExecution should exercise the automatic-policy path through handleExecution rather than manually skipping EmitApprovalSpan. Configure the test run or policy so isAutoApprovedByPolicy returns true, invoke handleExecution, and retain the assertion that the recorder contains no agenticrun.human_approval span.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/agenticolsconfig_types.go`:
- Around line 62-67: Regenerate the CRD manifests after adding the
AuditConfig.Enabled field by running the repository’s make manifests target,
then include the generated schema updates for spec.audit.enabled in the change.
Ensure the manifest reflects the field’s optional boolean type and default true
behavior.
In `@cmd/main.go`:
- Around line 46-47: Update the no-endpoint branch in the tracing setup and its
quickstart summary to state that remote OTLP export is disabled while stdout
OTLP JSON exporting remains active; replace the misleading “OTEL tracing
disabled” messaging without changing exporter behavior.
In `@controller/agenticrun/audit.go`:
- Around line 208-240: The approval audit currently deduplicates and emits one
span per run instead of per stage. In controller/agenticrun/audit.go lines
208-240, update ProductionAuditLogger.EmitApprovalSpan to accept the stage
explicitly, include approval.stage, and deduplicate using the run UID together
with the stage/revision. In controller/agenticrun/handlers.go lines 276-277,
invoke the stage-aware method for each manually approved stage and skip
policy-automatic decisions.
- Around line 63-68: The ProductionAuditLogger must not rely on process-local
priorPhase, emittedTerminal, emittedApproval, or knownUIDs for audit integrity.
Persist phase correlation, approval/terminal deduplication, and UID cleanup
state in the audited resource or another durable store, using these sync.Map
fields only as bounded caches if needed; update the related logic in
ProductionAuditLogger, including the referenced span emission and cleanup paths,
so restarts preserve links and prevent duplicates while retained completed
resources do not cause unbounded memory growth.
- Around line 160-203: Update startPhaseSpan and each StartAnalysisSpan,
StartExecutionSpan, StartVerificationSpan, and StartEscalationSpan method to
accept and pass the caller’s context into l.tracer.Start instead of using
context.Background(). Preserve the fresh phase-root behavior by adding
trace.WithNewRoot() to the span options if required, while retaining existing
attributes and span links.
In `@hack/quickstart/install.sh`:
- Line 414: Update the comment describing audit tracing to call the exporter
“stdout JSON” instead of “stdout OTLP JSON,” while preserving the existing note
that no remote collector is used.
---
Nitpick comments:
In `@controller/agenticrun/audit_test.go`:
- Around line 770-787: The test TestNoApprovalSpan_AutoApproveExecution should
exercise the automatic-policy path through handleExecution rather than manually
skipping EmitApprovalSpan. Configure the test run or policy so
isAutoApprovedByPolicy returns true, invoke handleExecution, and retain the
assertion that the recorder contains no agenticrun.human_approval span.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cf316376-6503-4695-89e3-154962093633
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (14)
api/v1alpha1/agenticolsconfig_types.goapi/v1alpha1/agenticolsconfig_types_test.gocmd/main.gocontroller/agenticrun/approval.gocontroller/agenticrun/audit.gocontroller/agenticrun/audit_test.gocontroller/agenticrun/bare_pod_manager_test.gocontroller/agenticrun/handlers.gocontroller/agenticrun/podspec_builder.gocontroller/agenticrun/reconciler.gocontroller/agenticrun/sandbox_templates.gogo.modhack/quickstart/install.shhack/quickstart/setup-audit.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
a07c252 to
906a1c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/agenticolsconfig_types.go`:
- Around line 68-81: Rename AuditConfig.Logging to an explicitly defaulted
Enabled field so the API matches the documented spec.audit.enabled contract, and
update LoggingEnabled() plus all related references to use it. Regenerate the
CRD manifests after changing the API type, ensuring the default and optional
validation markers are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2e6b1e93-ee36-413f-a03b-9691978f1f19
⛔ Files ignored due to path filters (2)
config/crd/bases/agentic.openshift.io_agenticolsconfigs.yamlis excluded by!config/crd/bases/**go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (12)
api/v1alpha1/agenticolsconfig_types.goapi/v1alpha1/agenticolsconfig_types_test.gocmd/main.gocontroller/agenticrun/approval.gocontroller/agenticrun/audit.gocontroller/agenticrun/audit_test.gocontroller/agenticrun/bare_pod_manager_test.gocontroller/agenticrun/handlers.gocontroller/agenticrun/reconciler.gogo.modhack/quickstart/install.shhack/quickstart/setup-audit.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
🚧 Files skipped from review as they are similar to previous changes (9)
- go.mod
- controller/agenticrun/reconciler.go
- hack/quickstart/install.sh
- cmd/main.go
- hack/quickstart/setup-audit.sh
- controller/agenticrun/approval.go
- controller/agenticrun/handlers.go
- controller/agenticrun/audit.go
- controller/agenticrun/audit_test.go
906a1c9 to
69a1e78
Compare
69a1e78 to
da6a0a6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controller/agenticrun/audit_test.go (1)
603-619: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUses deprecated
InstrumentationLibrary()/instrumentation.Library.
ReadOnlySpan.InstrumentationLibrary()is deprecated in favor ofInstrumentationScope()(returninginstrumentation.Scope) as of the OTel Go SDK. It still works at the pinnedv1.44.0, but consider migrating this assertion toInstrumentationScope()to avoid carrying deprecated-API usage forward.♻️ Proposed migration
- instrLib := spans[0].InstrumentationLibrary() - if instrLib.Name != tracerName { - t.Errorf("Expected instrumentation library name=%s, got %s", tracerName, instrLib.Name) - } - if instrLib.Version != tracerVersion { - t.Errorf("Expected instrumentation library version=%s, got %s", tracerVersion, instrLib.Version) - } + scope := spans[0].InstrumentationScope() + if scope.Name != tracerName { + t.Errorf("Expected instrumentation scope name=%s, got %s", tracerName, scope.Name) + } + if scope.Version != tracerVersion { + t.Errorf("Expected instrumentation scope version=%s, got %s", tracerVersion, scope.Version) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/agenticrun/audit_test.go` around lines 603 - 619, Update TestSpanInstrumentationLibrary to use ReadOnlySpan.InstrumentationScope() instead of the deprecated InstrumentationLibrary() method, and read the existing Name and Version assertions from the returned scope while preserving the current expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@controller/agenticrun/audit_test.go`:
- Around line 603-619: Update TestSpanInstrumentationLibrary to use
ReadOnlySpan.InstrumentationScope() instead of the deprecated
InstrumentationLibrary() method, and read the existing Name and Version
assertions from the returned scope while preserving the current expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ad6b6184-c0b2-4ac9-a464-0c17e6e33b57
⛔ Files ignored due to path filters (2)
config/crd/bases/agentic.openshift.io_agenticolsconfigs.yamlis excluded by!config/crd/bases/**go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (12)
api/v1alpha1/agenticolsconfig_types.goapi/v1alpha1/agenticolsconfig_types_test.gocmd/main.gocontroller/agenticrun/approval.gocontroller/agenticrun/audit.gocontroller/agenticrun/audit_test.gocontroller/agenticrun/bare_pod_manager_test.gocontroller/agenticrun/handlers.gocontroller/agenticrun/reconciler.gogo.modhack/quickstart/install.shhack/quickstart/setup-audit.sh
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
🚧 Files skipped from review as they are similar to previous changes (9)
- controller/agenticrun/approval.go
- hack/quickstart/setup-audit.sh
- cmd/main.go
- controller/agenticrun/bare_pod_manager_test.go
- api/v1alpha1/agenticolsconfig_types.go
- controller/agenticrun/reconciler.go
- hack/quickstart/install.sh
- controller/agenticrun/handlers.go
- controller/agenticrun/audit.go
fa34992 to
94e8c4d
Compare
onmete
left a comment
onmete
left a comment
There was a problem hiding this comment.
OLS-3536 adversarial review — request changes (score 36/100)
Scored against OLS-3536 + .ai/spec/what/audit-logging.md (Jira Spec Reference). Jira AC names proposal.*; scored against operator spec agenticrun.*.
AC summary
| # | Criterion | Status |
|---|---|---|
| 1 | Per-phase fresh auto-generated trace ID | PASS |
| 2 | agenticrun.uid/name/namespace on every span |
FAIL — uid keeps hyphens (spec wants 32-char hex) |
| 3 | Span Links to prior phase root | FAIL — in-process only; not persisted for restart |
| 4 | All spans kind INTERNAL |
PASS |
| 5 | CR split model (key fields + full CR) | PASS |
| 6 | Custom JSON → stdout OTLP JSON | FAIL — PrettyPrint SDK JSON; still audit.logging enum |
Must-fix (inline)
- Strip hyphens from
agenticrun.uid(and fix tests that assert the wrong form) - Persist prior phase span context (+ terminal/approval idempotency) on CR status/annotations
- Replace
stdouttrace.WithPrettyPrint()with real OTLP JSON wire format - Migrate to
spec.audit.enabled(*bool, default true) as claimed in the PR summary / spec rule 24 - Remove
truncateAttron compliance event attributes (spec: no truncation)
Should-fix
- Emit
agenticrun.approval.completedfrom the mutating webhook (spec 17–19), not only reconciler - Include selected-option full text on approval event
- Drop or use dead
traceIDFromAgenticRun - Align PR summary with the actual API (
loggingvsenabled)
Full write-up: local .ols/reviews/OLS-3536.md.
| // runAttrs returns the standard span attributes for an AgenticRun. | ||
| func runAttrs(run *agenticv1alpha1.AgenticRun) []attribute.KeyValue { | ||
| return []attribute.KeyValue{ | ||
| attribute.String("agenticrun.uid", string(run.UID)), |
There was a problem hiding this comment.
AC2 / spec rule 3: agenticrun.uid MUST be the AgenticRun UID with hyphens stripped (32-char hex). This stores string(run.UID) with hyphens.
traceIDFromAgenticRun already strips hyphens but is unused — wire it into runAttrs (as a string attribute, not as the OTel trace ID). Unit tests currently assert the hyphenated form and need updating.
| lifecycleSpans sync.Map // map[types.UID]*spanEntry | ||
| approvalSpans sync.Map // map[types.UID]*spanEntry | ||
| tracer trace.Tracer | ||
| priorPhase sync.Map // map[types.UID]trace.SpanContext |
There was a problem hiding this comment.
AC3 / spec rule 6: Span links to the prior phase require persisting the prior phase's span context (trace ID + span ID) on the AgenticRun status or annotations. An in-memory sync.Map is lost on operator restart, so mid-lifecycle runs lose the link chain.
Same problem for emittedTerminal / emittedApproval — restart can re-emit duplicates (matches the unchecked test-plan item).
| return string(data) | ||
| } | ||
|
|
||
| func truncateAttr(s string, max int) string { |
There was a problem hiding this comment.
Must-fix / spec rule 12: stdout compliance export MUST NOT truncate span or event attributes. truncateAttr is applied to request/summary/titles/descriptions. Drop truncation for audit event attributes (full fidelity in agenticrun.cr and key fields).
| } | ||
| for _, stage := range approval.Spec.Stages { | ||
| if stage.Type == agenticv1alpha1.ApprovalStageExecution && stage.Execution.Option != nil { | ||
| eventAttrs = append(eventAttrs, attribute.Int("selected_option", int(*stage.Execution.Option))) |
There was a problem hiding this comment.
Should-fix / spec §8 approval row: event should carry the selected option and its full text, not only selected_option index. Resolve the chosen AnalysisResult option text when emitting.
| // traceIDFromAgenticRun converts AgenticRun UID to OTEL trace ID. | ||
| // Strips hyphens from UID to produce 32 hex chars. | ||
| // traceIDFromAgenticRun converts AgenticRun UID to a trace.TraceID (for use as attribute value). | ||
| func traceIDFromAgenticRun(run *agenticv1alpha1.AgenticRun) trace.TraceID { |
There was a problem hiding this comment.
Nice-to-have: dead code after the lifecycle-span removal. Either use this for the hyphen-stripped agenticrun.uid attribute value, or delete it.
| sdktrace.WithIDGenerator(idGen), | ||
| ) | ||
| return tp, nil | ||
| stdoutExp, err := stdouttrace.New(stdouttrace.WithPrettyPrint()) |
There was a problem hiding this comment.
AC6 / spec rules 12–14, 25: stdouttrace.WithPrettyPrint() emits SDK diagnostic JSON (Name / SpanContext / …), not OTLP JSON (resourceSpans / ExportTraceServiceRequest). Log aggregators expecting OTLP JSON cannot consume this as specified.
Use an OTLP/HTTP JSON exporter (or equivalent) writing the OTLP wire format to stdout. Also gate exporters on audit enabled (rule 24).
| // Default: Enabled (when field is empty or config CR absent). | ||
| // +default="Enabled" | ||
| // +optional | ||
| Logging AuditLoggingMode `json:"logging,omitempty"` |
There was a problem hiding this comment.
AC6 / spec rule 24 + PR summary mismatch: Spec and the PR summary say spec.audit.enabled (*bool, default true) replaces the logging enum. This PR still exposes logging: Enabled|Disabled.
Either complete the API migration (with conversion/compatibility if needed) or correct the PR description / spec — do not claim the field was replaced.
| if r.Audit != nil { | ||
| r.Audit.EndApprovalWait(run, approval) | ||
| r.Audit.EmitApprovalReceived(ctx, run, approval) | ||
| r.Audit.EmitApprovalSpan(ctx, run, approval) |
There was a problem hiding this comment.
Should-fix / spec rules 17–19: agenticrun.approval.completed MUST be emitted by the mutating admission webhook on PATCH (same process / TracerProvider), not only from the reconciler when it observes the decision.
Reconciler emission can race with PATCH timing and diverges from the specified emission site. Keep webhook-authoritative approver fields; move span/event emission to the webhook path (reconciler may still skip auto-approved stages).
Response to review findingsThanks for the thorough review. Here's the disposition for each finding: Must-fix1. Strip hyphens from 2. Persist prior phase span context for restart (AC3) 3. Replace *4. Migrate to 5. Remove Should-fixEmit Include selected-option full text on approval event Drop or use dead Align PR summary with actual API |
7d36701 to
25af2d0
Compare
Replace the single lifecycle root span model with per-phase independent traces. Each phase (analyze, execute, verify, escalate, approval, terminal) gets its own auto-generated trace ID. Span links connect consecutive phases for correlation. Key changes: - AuditLogger interface: per-phase Start*Span methods, EmitApprovalSpan, EmitTerminalSpan, Cleanup; remove lifecycle span and approval wait APIs - ProductionAuditLogger: tracer + sync.Map for prior phase span context; no more IDGenerator, zap logger, or in-memory span maps - NoOpAuditLogger: uses noop.TracerProvider for valid (non-nil) spans - Config: spec.audit.enabled (*bool, default true) replaces spec.audit.logging enum - main.go: always-on stdout OTLP JSON exporter alongside optional OTLP - All caller sites updated in reconciler.go and handlers.go Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Vimal Kumar <vimal78@gmail.com>
25af2d0 to
754a5cc
Compare
|
@vimalk78: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Adversarial Review: OLS-3536 Replace lifecycle span model with per-phase independent tracesCross-referenced against Verdict: Approve with one caveatOne pre-existing issue carried forward — Must-Fix
Sandbox spec rule 8a: content capture MUST be opt-in. There is no way to enable audit without also capturing PII-bearing LLM output (gen_ai.completion, gen_ai.reasoning_content). The parent spec marks content capture controls as [DEFERRED: needs Jira]. Fix options: (a) gate on a new CRD field spec.audit.captureContent, or (b) remove LIGHTSPEED_CAPTURE_CONTENT from both files so it defaults to false until the control is implemented. Option (b) is the safe default — audit works without content capture; content capture without user consent is the compliance risk. Vimal: Content capture is deliberately set to full till we do refinement for content capture and update specs |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: onmete The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
2 participants
Summary
CleanupDeletedto prevent sync.Map memory leak when runs are deletedhuman_approvalspan emission (checks ApprovalPolicy directly)spec.audit.enabled(bool, default true) replacesspec.audit.loggingenumTest plan
make test— all unit tests passmake fmt && make vet— clean🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com