feat(observability): connect event bus components - #91
Conversation
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAuth-callout now supports direct OTLP export by default and optional OpenTelemetry Operator injection. NACK metrics monitoring resources and scrape timeouts were added. Local OpenTelemetry deployment wiring, documentation, and functional observability checks were updated. ChangesObservability deployment and validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Skaffold
participant OTelOperator
participant OTelCollector
participant AuthCallout
participant Prometheus
participant NACK
Skaffold->>OTelOperator: Deploy operator configuration
Skaffold->>OTelCollector: Deploy collector configuration
OTelOperator->>AuthCallout: Inject tracing configuration when enabled
AuthCallout->>OTelCollector: Export OTLP/gRPC traces
Prometheus->>AuthCallout: Scrape /metrics with configured timeout
Prometheus->>NACK: Scrape /metrics with configured timeout
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 2
🧹 Nitpick comments (1)
deploy/nats-event-bus/templates/nack-monitoring.yaml (1)
22-23: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep the metrics Service selector synchronized with NACK label mode.
The selector is hard-coded to
app: nack. The upstream NACK chart supports switching between legacyapplabels and modernapp.kubernetes.io/*labels; with the modern mode, this Service can have no endpoints. Derive the selector from the pinned chart helper or condition it on the selected label mode, then render both modes withhelm lint. (raw.githubusercontent.com)🤖 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/nats-event-bus/templates/nack-monitoring.yaml` around lines 22 - 23, Update the metrics Service selector in nack-monitoring.yaml to follow the pinned NACK chart’s selected label mode instead of hard-coding app: nack. Reuse the chart helper or conditionally render the corresponding legacy app labels and modern app.kubernetes.io labels, and verify both modes with helm lint.Source: MCP tools
🤖 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 `@auth-callout/deploy/values.yaml`:
- Line 408: Align the Auth Callout scrape contract by changing the default
interval to 30s in auth-callout/deploy/values.yaml at lines 408-408 and updating
the example interval to 30s in auth-callout/deploy/README.md at lines 280-280;
keep the 10s scrape timeout unchanged.
In `@skaffold.yaml`:
- Around line 9-11: Update the event-bus configuration in skaffold.yaml to add
../agent-gateway/skaffold.fixtures.yaml with config agent-gateway-fixtures to
its requires list, ensuring the agent-gateway fixtures deploy before event-bus.
---
Nitpick comments:
In `@deploy/nats-event-bus/templates/nack-monitoring.yaml`:
- Around line 22-23: Update the metrics Service selector in nack-monitoring.yaml
to follow the pinned NACK chart’s selected label mode instead of hard-coding
app: nack. Reuse the chart helper or conditionally render the corresponding
legacy app labels and modern app.kubernetes.io labels, and verify both modes
with helm lint.
🪄 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: 1f05ea32-7d78-4b91-8bea-58091821def3
📒 Files selected for processing (10)
auth-callout/deploy/README.mdauth-callout/deploy/templates/deployment.yamlauth-callout/deploy/templates/servicemonitor.yamlauth-callout/deploy/values.yamldeploy/README.mddeploy/nats-event-bus/templates/nack-monitoring.yamldeploy/nats-event-bus/values.yamllocal/agent-gateway/tests/functional/session_protocol_health_test.golocal/event-bus/k8s/local-dev-values.yamlskaffold.yaml
💤 Files with no reviewable changes (1)
- local/event-bus/k8s/local-dev-values.yaml
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
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 `@auth-callout/deploy/values.yaml`:
- Around line 258-265: Update the default observability tracing configuration in
values.yaml so tracing is disabled when operatorInjection.enabled is false, or
configure an explicit non-sidecar exporter endpoint. Keep the default settings
consistent so a chart install does not target the unavailable 127.0.0.1:4317
collector.
🪄 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: 0cde1490-c435-4750-953a-ee48966b78a8
📒 Files selected for processing (5)
auth-callout/deploy/README.mdauth-callout/deploy/templates/deployment.yamlauth-callout/deploy/values.yamldeploy/nats-event-bus/values.yamllocal/event-bus/skaffold.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- auth-callout/deploy/README.md
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
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 `@auth-callout/deploy/values.yaml`:
- Around line 262-264: Update the OpenTelemetry configuration around
instrumentationRef and sidecarRef so direct Helm deployments either document and
provision the required dsx-obs/default-instrumentation and
dsx-obs/default-sidecar resources first, or disable injection when those
resources are unavailable; preserve the current enabled behavior when the
prerequisites exist.
🪄 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: 708e1825-86d2-4cba-83c3-83f78fdc991f
📒 Files selected for processing (3)
auth-callout/deploy/README.mdauth-callout/deploy/values.yamldeploy/nats-event-bus/values.yaml
💤 Files with no reviewable changes (1)
- deploy/nats-event-bus/values.yaml
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-31 03:25:29 UTC | Commit: 7705d98 |
🛡️ CodeQL Analysis🚨 Found 1 issue(s) Severity Breakdown:
📋 Top Issues💡 Note: Enable GitHub Advanced Security to see full details in the Security tab. 🕐 Last updated: 2026-07-31 03:27:12 UTC | Commit: 7705d98 |
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-91.docs.buildwithfern.com/dsx-exchange |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
local/agent-gateway/tests/functional/session_protocol_health_test.go (1)
345-345: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGate the
auth-calloutcase on operator injection.Skip the case unless
auth-callout.observability.tracing.operatorInjection.enabledistrue. The chart omits the injection annotations and sidecar when this value isfalse, so the unconditional assertions fail in valid direct-export deployments.🤖 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 `@local/agent-gateway/tests/functional/session_protocol_health_test.go` at line 345, Update the “auth callout” case in the session protocol health tests to run only when auth-callout operator injection is enabled via auth-callout.observability.tracing.operatorInjection.enabled. Preserve the existing assertions for injected deployments while skipping this case for direct-export configurations where injection is disabled.
🤖 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 `@local/agent-gateway/tests/functional/session_protocol_health_test.go`:
- Around line 269-279: Extend the relabeling validation in the nack branch of
the session protocol health test to inspect relabeling["sourceLabels"] and
assert it contains exactly "__meta_kubernetes_pod_ip", alongside the existing
targetLabel and replacement checks.
- Around line 244-248: Update the pod selector validation in the podMonitor
branch to pass a status.phase=Running filter to runner.ListPods. Keep the
existing empty-match failure behavior and diagnostic message, while ensuring
completed or failed Pods do not satisfy the selector check.
- Around line 190-193: Update the NACK metrics probe around firstRunningPodName
to retrieve the rendered PodMonitor/nack spec.selector.matchLabels and pass that
complete label set to firstRunningPodName instead of hardcoding app=nack.
Preserve the existing metrics endpoint validation after selecting the pod.
---
Outside diff comments:
In `@local/agent-gateway/tests/functional/session_protocol_health_test.go`:
- Line 345: Update the “auth callout” case in the session protocol health tests
to run only when auth-callout operator injection is enabled via
auth-callout.observability.tracing.operatorInjection.enabled. Preserve the
existing assertions for injected deployments while skipping this case for
direct-export configurations where injection is disabled.
🪄 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: 18b91be9-37a4-4aa7-a62e-23ead56803f0
📒 Files selected for processing (8)
deploy/README.mddeploy/nats-event-bus/templates/nack-monitoring.yamldeploy/nats-event-bus/values.yamllocal/agent-gateway/skaffold.fixtures.yamllocal/agent-gateway/skaffold.infra.yamllocal/agent-gateway/tests/functional/session_protocol_health_test.golocal/event-bus/k8s/local-dev-values.yamllocal/event-bus/skaffold.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- local/event-bus/skaffold.yaml
- local/agent-gateway/skaffold.fixtures.yaml
- local/agent-gateway/skaffold.infra.yaml
- deploy/nats-event-bus/values.yaml
- deploy/README.md
- local/event-bus/k8s/local-dev-values.yaml
Signed-off-by: Frank Spitulski <fspitulski@nvidia.com>
2 participants
Summary
Component inventory
Agent Gateway dataplane, controller, rate-limit service, Valkey exporter, and
bridge were already connected and remain covered by the shared live
observability tests.
Validation
make checkhelm lint auth-callout/deployhelm lint deploy/nats-event-busmake testSummary by CodeRabbit
New Features
Documentation
Improvements