Orc metrics - #87
Merged
Merged
Conversation
jessicaochen
requested review from
Edwinhr716 and
lynnl0927
and removed request for
aishukamal and
lynnl0927
July 9, 2026 19:23
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughAdds Prometheus metrics definitions and registration, instruments controller and server operations, exposes a ChangesPrometheus metrics instrumentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Main as acceleratororchestrator main
participant Server as StartServer
participant Controller as Controller
participant Metrics as metrics package
participant Prometheus as Prometheus scraper
Main->>Metrics: Register metrics
Main->>Server: StartServer with metricsPort
Server->>Server: Serve /metrics
Controller->>Metrics: Record queue and operation metrics
Server->>Metrics: Record acquire and deferred snapshot metrics
Prometheus->>Server: GET /metrics
Server-->>Prometheus: Prometheus metrics
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (3)
pkg/accelerator-orchestrator/server/server.go (1)
293-298: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMetrics server bind failure is only logged, not surfaced.
If
httpServer.ListenAndServe()fails (e.g. port conflict), the error is logged but the process keeps running with gRPC serving normally while/metricssilently never comes up. Given metrics-port is now a required operational contract (Helm exposes it, scraping depends on it), consider surfacing this failure more visibly (e.g., via a startup health signal) rather than only a log line.🤖 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 `@pkg/accelerator-orchestrator/server/server.go` around lines 293 - 298, The metrics server startup path in the anonymous goroutine around httpServer.ListenAndServe only logs bind failures and lets the process continue, so surface this as a startup-critical failure instead of a log-only event. Update the server startup flow in server.go to propagate or signal the error from ListenAndServe (for example via the main startup/health coordination used by the server) and make sure the failure is visible to the orchestrator rather than leaving gRPC running with metrics silently unavailable.pkg/accelerator-orchestrator/server/server_test.go (1)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGlobal metric assertion is order/isolation-dependent.
testutil.CollectAndCount(metrics.AcquireWaitDuration)checks a package-level singleton shared across all test cases/files in the binary. The== 0check is tolerant to prior accumulation, but it can't detect regressions where this specific call fails to observe (since other tests/paths may have already incremented it). This is an inherent limitation of testing global Prometheus collectors without a per-test registry; acceptable here given the loose assertion, but worth keeping in mind if stricter counts are needed later.🤖 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 `@pkg/accelerator-orchestrator/server/server_test.go` around lines 108 - 110, The assertion on metrics.AcquireWaitDuration is using the global Prometheus collector, so it cannot reliably prove this test case recorded a new observation. In server_test.go, either keep the loose presence check as-is and avoid tightening it, or switch the test to a per-test registry/isolated collector setup around the AcquireWaitDuration observation path so the check is scoped to this test instead of shared process-wide state.deploy/acceleratororchestrator/templates/deployment.yaml (1)
27-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMetrics port is hardcoded; consider making it configurable via
.Values.The
grpcport on line 25 (50051) is also hardcoded, so this is consistent with existing chart style, but the gRPC service port on the Service side is templated via.Values.service.portwhile metrics is not on either side. Sincemain.goexposes--metrics-portas a configurable flag (default 8080), hardcoding it in the chart means operators can't change it without editing templates directly.♻️ Proposed refactor
- name: metrics - containerPort: 8080 + containerPort: {{ .Values.metrics.port | default 8080 }} protocol: TCP🤖 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 `@deploy/acceleratororchestrator/templates/deployment.yaml` around lines 27 - 29, The metrics container port in the deployment template is hardcoded, so wire it through chart values instead of fixing it at 8080. Update the deployment spec that defines the metrics port to read from .Values, and keep it aligned with the configurable --metrics-port flag exposed by main.go. Use the existing port templating pattern already used for the service port (for example, the service.port value) and add a matching metrics value if needed.
🤖 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 `@pkg/accelerator-orchestrator/server/server.go`:
- Around line 285-292: Set the metrics HTTP server in server.go to use a full
timeout configuration, not just ReadHeaderTimeout. Update the http.Server
initialization in the metrics server setup to also define ReadTimeout,
WriteTimeout, and IdleTimeout alongside the existing ReadHeaderTimeout so slow
or malicious clients cannot keep connections open indefinitely. Use the
httpServer construction near the "/metrics" mux setup as the place to apply
these additional timeout fields.
---
Nitpick comments:
In `@deploy/acceleratororchestrator/templates/deployment.yaml`:
- Around line 27-29: The metrics container port in the deployment template is
hardcoded, so wire it through chart values instead of fixing it at 8080. Update
the deployment spec that defines the metrics port to read from .Values, and keep
it aligned with the configurable --metrics-port flag exposed by main.go. Use the
existing port templating pattern already used for the service port (for example,
the service.port value) and add a matching metrics value if needed.
In `@pkg/accelerator-orchestrator/server/server_test.go`:
- Around line 108-110: The assertion on metrics.AcquireWaitDuration is using the
global Prometheus collector, so it cannot reliably prove this test case recorded
a new observation. In server_test.go, either keep the loose presence check as-is
and avoid tightening it, or switch the test to a per-test registry/isolated
collector setup around the AcquireWaitDuration observation path so the check is
scoped to this test instead of shared process-wide state.
In `@pkg/accelerator-orchestrator/server/server.go`:
- Around line 293-298: The metrics server startup path in the anonymous
goroutine around httpServer.ListenAndServe only logs bind failures and lets the
process continue, so surface this as a startup-critical failure instead of a
log-only event. Update the server startup flow in server.go to propagate or
signal the error from ListenAndServe (for example via the main startup/health
coordination used by the server) and make sure the failure is visible to the
orchestrator rather than leaving gRPC running with metrics silently unavailable.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8745f144-0a3b-4dea-a843-164bd2c409cd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/acceleratororchestrator/main.godeploy/acceleratororchestrator/templates/deployment.yamldeploy/acceleratororchestrator/templates/service.yamlgo.modpkg/accelerator-orchestrator/controller/controller.gopkg/accelerator-orchestrator/controller/controller_internal_test.gopkg/accelerator-orchestrator/controller/controller_test.gopkg/accelerator-orchestrator/metrics/metrics.gopkg/accelerator-orchestrator/server/server.gopkg/accelerator-orchestrator/server/server_test.go
aishukamal
requested changes
Jul 15, 2026
github-project-automation
Bot
moved this from Todo
to In Progress
in Time-Slicing Project Tracker
Collaborator
|
/approve |
aishukamal
self-requested a review
July 16, 2026 18:41
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
jessicaochen
force-pushed
the
orc-metrics
branch
from
July 16, 2026 19:19
fdf4647 to
70c4146
Compare
aishukamal
approved these changes
Jul 16, 2026
github-project-automation
Bot
moved this from In Progress
to Done
in Time-Slicing Project Tracker
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
2 participants
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.
What does this PR do?
MVP set of metrics for the orchestrator.
Why is this change needed?
How was this tested?
Checklist
git commit -s) per DCOmake test)make lint)Related Issues
Summary by CodeRabbit
/metricsendpoint on a configurable--metrics-port(default 8080), including Kubernetes service/deployment updates.