OLS-2989: Implement operator-side metrics envelope parsing and Result CR storage#133
OLS-2989: Implement operator-side metrics envelope parsing and Result CR storage#133blublinsky wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@blublinsky: This pull request references OLS-2989 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
More reviews will be available in 13 minutes and 16 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughAdds ChangesMetrics Collection and Storage
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/proposal/results.go (1)
290-320:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd Metrics field copy to all four Result types.
copyResultStatusupdates existing Result CRs on AlreadyExists but does not copy theMetricsfield. WhencreateIdempotentencounters an existing CR (e.g., reconcile retry), the metrics from the latest agent response are lost.Add
d.Status.Metrics = s.Status.Metricsin each case block to persist metrics consistently.🔧 Proposed fix
func copyResultStatus(dst, src client.Object) { switch d := dst.(type) { case *agenticv1alpha1.AnalysisResult: if s, ok := src.(*agenticv1alpha1.AnalysisResult); ok { d.Status.Options = s.Status.Options d.Status.FailureReason = s.Status.FailureReason d.Status.Sandbox = s.Status.Sandbox + d.Status.Metrics = s.Status.Metrics } case *agenticv1alpha1.ExecutionResult: if s, ok := src.(*agenticv1alpha1.ExecutionResult); ok { d.Status.ActionsTaken = s.Status.ActionsTaken d.Status.Verification = s.Status.Verification d.Status.FailureReason = s.Status.FailureReason d.Status.Sandbox = s.Status.Sandbox + d.Status.Metrics = s.Status.Metrics } case *agenticv1alpha1.VerificationResult: if s, ok := src.(*agenticv1alpha1.VerificationResult); ok { d.Status.Checks = s.Status.Checks d.Status.Summary = s.Status.Summary d.Status.FailureReason = s.Status.FailureReason d.Status.Sandbox = s.Status.Sandbox + d.Status.Metrics = s.Status.Metrics } case *agenticv1alpha1.EscalationResult: if s, ok := src.(*agenticv1alpha1.EscalationResult); ok { d.Status.Summary = s.Status.Summary d.Status.Content = s.Status.Content d.Status.FailureReason = s.Status.FailureReason d.Status.Sandbox = s.Status.Sandbox + d.Status.Metrics = s.Status.Metrics } } }🤖 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/proposal/results.go` around lines 290 - 320, The copyResultStatus function copies various status fields for all four Result types (AnalysisResult, ExecutionResult, VerificationResult, EscalationResult) but omits the Metrics field, causing metrics data to be lost when updating existing CRs during reconcile retries. Add d.Status.Metrics = s.Status.Metrics to each of the four case blocks in copyResultStatus (the AnalysisResult case, ExecutionResult case, VerificationResult case, and EscalationResult case) to ensure metrics are consistently persisted alongside the other status fields.
🧹 Nitpick comments (1)
test/agent/main.go (1)
154-163: 💤 Low valueMock envelope construction is functional but consider structured marshaling.
The envelope is built via string formatting rather than marshaling a struct. This works since
cannedResponsereturns valid JSON that can be directly embedded, but usingjson.Marshalon a struct would be more maintainable if response formats evolve.♻️ Optional refactor for type safety
- result := cannedResponse(phase, ns) - - envelope := fmt.Sprintf(`{"metrics":{"latency_ms":1500,"input_tokens":100,"output_tokens":50,"model":"mock-model","provider":"mock","tool_calls_count":1},"result":%s}`, string(result)) + result := cannedResponse(phase, ns) + + envelope := map[string]interface{}{ + "metrics": map[string]interface{}{ + "latency_ms": 1500, + "input_tokens": 100, + "output_tokens": 50, + "model": "mock-model", + "provider": "mock", + "tool_calls_count": 1, + }, + "result": json.RawMessage(result), + } + envelopeJSON, err := json.Marshal(envelope) + if err != nil { + log.Printf("marshal envelope: %v", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) - if _, err := w.Write([]byte(envelope)); err != nil { + if _, err := w.Write(envelopeJSON); err != nil { log.Printf("write response: %v", err) }🤖 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 `@test/agent/main.go` around lines 154 - 163, The envelope in the mock response handler is constructed using fmt.Sprintf with a hardcoded JSON string template, which reduces maintainability if response formats evolve. Instead, define a struct type that represents the response envelope with fields for metrics (containing latency_ms, input_tokens, output_tokens, model, provider, tool_calls_count) and result, then use json.Marshal to serialize this struct with the actual values from cannedResponse and the metrics data. This approach provides better type safety and makes future format changes easier to manage.
🤖 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 @.ai/spec/how/reconciler.md:
- Around line 138-140: The RunMetrics documentation in reconciler.md at line 140
incorrectly documents CostUSD as `*float64`, but the actual implementation uses
`*string` validated with regex pattern `^\d+(\.\d+)?$`. Update the CostUSD field
documentation to reflect the correct type as `*string` and note that it is
validated with the regex pattern to maintain consistency with the actual
implementation that uses this approach for cross-language portability.
In `@api/v1alpha1/shared_types.go`:
- Around line 198-208: Add non-negative validation constraints to the metric
counter and latency fields in the shared_types.go file. For the fields
LatencyMs, InputTokens, OutputTokens, and the additional field at line 228 (as
noted in the comment), add a kubebuilder validation marker (such as
`+kubebuilder:validation:Minimum=0`) above each field definition to enforce that
these fields can only accept zero or positive integer values, preventing invalid
telemetry data with negative values from being stored in status.
In `@controller/proposal/client.go`:
- Around line 149-151: The nil check on the response.Result field in the
validation block only rejects missing fields but allows null values to pass
through, meaning a response with `{"result":null}` would silently proceed to
downstream unmarshalling. Modify the validation logic to explicitly reject cases
where the result field is explicitly set to null, not just missing, by adding an
additional check after the nil validation or by using a pointer field that
distinguishes between absent and null values during JSON unmarshalling.
- Around line 145-153: The code enforces that the agentRunResponse must contain
a non-nil result field, but the current sandbox agent returns a legacy format
with success and summary fields instead. To support mixed-version deployments,
add a fallback parsing mechanism: after unmarshaling into agentRunResponse,
check if result is nil, and if so, attempt to parse the response body into a
legacy response structure (with success and summary fields). Convert the legacy
format to the envelope format or handle it separately, ensuring backward
compatibility before raising an error for missing result field.
---
Outside diff comments:
In `@controller/proposal/results.go`:
- Around line 290-320: The copyResultStatus function copies various status
fields for all four Result types (AnalysisResult, ExecutionResult,
VerificationResult, EscalationResult) but omits the Metrics field, causing
metrics data to be lost when updating existing CRs during reconcile retries. Add
d.Status.Metrics = s.Status.Metrics to each of the four case blocks in
copyResultStatus (the AnalysisResult case, ExecutionResult case,
VerificationResult case, and EscalationResult case) to ensure metrics are
consistently persisted alongside the other status fields.
---
Nitpick comments:
In `@test/agent/main.go`:
- Around line 154-163: The envelope in the mock response handler is constructed
using fmt.Sprintf with a hardcoded JSON string template, which reduces
maintainability if response formats evolve. Instead, define a struct type that
represents the response envelope with fields for metrics (containing latency_ms,
input_tokens, output_tokens, model, provider, tool_calls_count) and result, then
use json.Marshal to serialize this struct with the actual values from
cannedResponse and the metrics data. This approach provides better type safety
and makes future format changes easier to manage.
🪄 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: f88df944-d6fc-4694-b8ed-d0612f147d7a
⛔ Files ignored due to path filters (4)
config/crd/bases/agentic.openshift.io_analysisresults.yamlis excluded by!config/crd/bases/**config/crd/bases/agentic.openshift.io_escalationresults.yamlis excluded by!config/crd/bases/**config/crd/bases/agentic.openshift.io_executionresults.yamlis excluded by!config/crd/bases/**config/crd/bases/agentic.openshift.io_verificationresults.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (15)
.ai/spec/how/reconciler.md.ai/spec/what/sandbox-execution.mdapi/v1alpha1/analysisresult_types.goapi/v1alpha1/escalationresult_types.goapi/v1alpha1/executionresult_types.goapi/v1alpha1/shared_types.goapi/v1alpha1/verificationresult_types.gocontroller/proposal/agent.gocontroller/proposal/client.gocontroller/proposal/client_test.gocontroller/proposal/reconciler_test.gocontroller/proposal/results.gocontroller/proposal/sandbox_agent.gocontroller/proposal/sandbox_agent_test.gotest/agent/main.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
| var response agentRunResponse | ||
| if err := json.Unmarshal(respBody, &response); err != nil { | ||
| return nil, fmt.Errorf("parse response envelope: %w", err) | ||
| } | ||
| if response.Result == nil { | ||
| return nil, fmt.Errorf("response envelope missing 'result' field") | ||
| } | ||
|
|
||
| return &response, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the full context of the client.go file around lines 145-153
wc -l controller/proposal/client.goRepository: openshift/lightspeed-agentic-operator
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Read the full response parsing and agentRunResponse struct definition
sed -n '1,200p' controller/proposal/client.go | cat -nRepository: openshift/lightspeed-agentic-operator
Length of output: 5978
🏁 Script executed:
#!/bin/bash
# Search for agentRunResponse struct definition
rg -A 10 "type agentRunResponse struct"Repository: openshift/lightspeed-agentic-operator
Length of output: 862
🏁 Script executed:
#!/bin/bash
# Check if there's any fallback or version detection logic
rg -n "success\|summary\|legacy\|fallback\|version" controller/proposal/client.go | head -30Repository: openshift/lightspeed-agentic-operator
Length of output: 63
🏁 Script executed:
#!/bin/bash
# Check for any deployment coordination or version detection logic
rg -n "Metrics\|Result\|envelope" controller/proposal/client.go | head -20Repository: openshift/lightspeed-agentic-operator
Length of output: 63
🏁 Script executed:
#!/bin/bash
# Check where the response is used after parsing - does code handle nil Metrics?
rg -A 5 -B 5 "agentRunResponse" controller/proposal/reconciler.go | head -50Repository: openshift/lightspeed-agentic-operator
Length of output: 63
🏁 Script executed:
#!/bin/bash
# Verify current agent response format to confirm it's legacy
grep -r "success\|summary" src/lightspeed_agentic/routes/models.py | head -20Repository: openshift/lightspeed-agentic-sandbox
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Find where Run() result is used and how response is processed
rg -n "\.Run\(" controller/proposal/Repository: openshift/lightspeed-agentic-operator
Length of output: 3885
🏁 Script executed:
#!/bin/bash
# Search more broadly for response handling
find controller/proposal -name "*.go" -exec grep -l "agentRunResponse\|response\." {} \;Repository: openshift/lightspeed-agentic-operator
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Check the full reconciler to see response usage
wc -l controller/proposal/reconciler.go
sed -n '1,50p' controller/proposal/reconciler.go | cat -nRepository: openshift/lightspeed-agentic-operator
Length of output: 3109
🏁 Script executed:
#!/bin/bash
# Check sandbox_agent.go to see how the response is processed
sed -n '200,230p' controller/proposal/sandbox_agent.go | cat -nRepository: openshift/lightspeed-agentic-operator
Length of output: 1197
🏁 Script executed:
#!/bin/bash
# Look for broader response handling around line 213
sed -n '190,240p' controller/proposal/sandbox_agent.go | cat -nRepository: openshift/lightspeed-agentic-operator
Length of output: 1823
🏁 Script executed:
#!/bin/bash
# Check client_test.go to see what the test expects
cat -n controller/proposal/client_test.go | head -100Repository: openshift/lightspeed-agentic-operator
Length of output: 4165
Envelope parsing requires result field; missing compatibility with current agent's legacy format.
The code hard-requires a result field in the agent response (line 149 errors if missing). The current sandbox agent returns legacy {success, summary} format, not the envelope structure. Mixed-version deployments will fail at runtime when the client receives legacy responses and finds result empty.
The metrics field is optional (pointer), but the version-skew break is real. Confirm rollout coordination or add a legacy fallback parser before shipping independently.
🤖 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/proposal/client.go` around lines 145 - 153, The code enforces that
the agentRunResponse must contain a non-nil result field, but the current
sandbox agent returns a legacy format with success and summary fields instead.
To support mixed-version deployments, add a fallback parsing mechanism: after
unmarshaling into agentRunResponse, check if result is nil, and if so, attempt
to parse the response body into a legacy response structure (with success and
summary fields). Convert the legacy format to the envelope format or handle it
separately, ensuring backward compatibility before raising an error for missing
result field.
Source: Linked repositories
blublinsky
force-pushed
the
metric-envelope
branch
from
e39844f to
6a4f6fc
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/analysisresult_types.go`:
- Around line 53-55: The new Metrics field added to AnalysisResultStatus in the
AnalysisResult type is not being preserved when status updates occur. In the
copyResultStatus helper function in controller/proposal/results.go, locate the
branch that handles *AnalysisResult and add a line to copy the Metrics field
from the source Status to the destination Status, ensuring telemetry data is not
lost during status copy/update operations.
🪄 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: 4c4aa20f-9473-4b13-8ff7-60a0e7e53678
⛔ Files ignored due to path filters (4)
config/crd/bases/agentic.openshift.io_analysisresults.yamlis excluded by!config/crd/bases/**config/crd/bases/agentic.openshift.io_escalationresults.yamlis excluded by!config/crd/bases/**config/crd/bases/agentic.openshift.io_executionresults.yamlis excluded by!config/crd/bases/**config/crd/bases/agentic.openshift.io_verificationresults.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (15)
.ai/spec/how/reconciler.md.ai/spec/what/sandbox-execution.mdapi/v1alpha1/analysisresult_types.goapi/v1alpha1/escalationresult_types.goapi/v1alpha1/executionresult_types.goapi/v1alpha1/shared_types.goapi/v1alpha1/verificationresult_types.gocontroller/proposal/agent.gocontroller/proposal/client.gocontroller/proposal/client_test.gocontroller/proposal/reconciler_test.gocontroller/proposal/results.gocontroller/proposal/sandbox_agent.gocontroller/proposal/sandbox_agent_test.gotest/agent/main.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/lightspeed-agentic-sandbox(manual)
✅ Files skipped from review due to trivial changes (2)
- .ai/spec/how/reconciler.md
- controller/proposal/sandbox_agent_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- controller/proposal/reconciler_test.go
- test/agent/main.go
- api/v1alpha1/shared_types.go
- controller/proposal/results.go
- .ai/spec/what/sandbox-execution.md
- controller/proposal/client.go
- controller/proposal/sandbox_agent.go
- controller/proposal/agent.go
- controller/proposal/client_test.go
blublinsky
force-pushed
the
metric-envelope
branch
2 times, most recently
from
e87be40 to
356fff5
Compare
blublinsky
force-pushed
the
metric-envelope
branch
from
356fff5 to
79fc53a
Compare
|
@blublinsky: 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. |
2 participants
Summary
Implements the operator side of the sandbox → operator metrics handoff (companion to lightspeed-agentic-sandbox#74). The agent's POST /v1/agent/run response is now a {metrics, result} envelope. The operator parses the envelope, threads metrics through the call chain, and persists them on Result CR statuses.
Design decisions
StepMetrics on Result CR status (not Prometheus, not Proposal status): Result CRs are the source of truth per-step. Metrics live with the step's outcome. Prometheus/export can be added later by reading these fields.
CostUSD as string: controller-gen rejects float64 for cross-language portability. String with regex validation (^\d+(.\d+)?$) is the standard pattern.
toStepMetrics(nil) → nil: Failure paths (where agent didn't respond) don't produce empty metric blocks.
Envelope validation: Missing result field fails immediately with a clear error rather than silently producing nil output.