HYPERFLEET-1363 - feat: add Grafana dashboard provisioning to Helm chart - #268
HYPERFLEET-1363 - feat: add Grafana dashboard provisioning to Helm chart#268Ruclo wants to merge 1 commit into
Conversation
|
[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 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe Helm chart nests ServiceMonitor and tracing values under Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant HelmTemplates
participant Kubernetes
participant Grafana
HelmValues->>HelmTemplates: Provide monitoring configuration
HelmTemplates->>Kubernetes: Render ServiceMonitor and dashboard ConfigMap when enabled
Kubernetes->>Grafana: Expose dashboard ConfigMap
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 1 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 349 lines (>200) | +1 |
| Sensitive paths | none | +0 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@charts/Chart.yaml`:
- Line 5: Update the chart version from 2.2.0 to 3.0.0 in Chart.yaml to mark the
serviceMonitor.* and tracing.* to monitoring.* values migration as a
backward-incompatible API change.
In `@charts/values.yaml`:
- Around line 368-372: Update the serviceMonitor.enabled description in
charts/values.yaml (lines 368-372) to state that it defaults to disabled, then
regenerate the chart documentation so charts/README.md (lines 130-131) reflects
the corrected description.
In `@docs/adapter-authoring-guide.md`:
- Line 1772: Update the RabbitMQ example in the adapter authoring guide to use
the chart’s exchangeType key under broker.rabbitmq instead of exchange_type.
Keep the surrounding example unchanged and align the documented configuration
with the key that the chart actually reads.
- Around line 1782-1785: Update the RabbitMQ configuration example near the
adapter deployment command to remove the password from the `broker.rabbitmq.url`
`--set` argument. Demonstrate referencing a Kubernetes Secret-backed chart value
for the RabbitMQ credential while preserving the existing broker URL and
deployment configuration.
- Line 1770: Update the RabbitMQ URL example in the adapter authoring guide to
use the amqps:// scheme, and document the required CA or client-certificate
configuration alongside it. Keep the existing host, port, credentials, and
virtual-host placeholders intact.
In `@docs/deployment.md`:
- Around line 340-343: Update the monitoring.tracing deployment example to use
an https:// OTLP endpoint and document the required CA settings for TLS
validation; do not leave the example using an unencrypted http:// endpoint.
In `@docs/metrics.md`:
- Line 7: Update the ServiceMonitor configuration comment in values.yaml near
monitoring.serviceMonitor to state that provisioning is disabled by default,
matching enabled: false and the documentation. Leave the configuration value and
other ServiceMonitor options unchanged.
🪄 Autofix
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 452b468f-1c92-4425-98ec-ef110a8cf2f3
📒 Files selected for processing (13)
Makefilecharts/Chart.yamlcharts/README.mdcharts/templates/configmap-dashboard.yamlcharts/templates/deployment.yamlcharts/templates/servicemonitor.yamlcharts/values.schema.jsoncharts/values.yamldocs/adapter-authoring-guide.mddocs/configuration.mddocs/deployment.mddocs/metrics.mddocs/runbook.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
| description: HyperFleet Adapter - Event-driven adapter services for HyperFleet cluster provisioning | ||
| type: application | ||
| version: 2.1.0 | ||
| version: 2.2.0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Release this values API change as a major chart version.
This PR moves serviceMonitor.* and tracing.* under monitoring.*. Existing values files that use the old keys will silently stop enabling monitoring or tracing. Preserve the old keys with a compatibility shim, or release this change as 3.0.0 and document the migration. Semantic Versioning requires a major increment for backward-incompatible public API changes. (semver.org)
🤖 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 `@charts/Chart.yaml` at line 5, Update the chart version from 2.2.0 to 3.0.0 in
Chart.yaml to mark the serviceMonitor.* and tracing.* to monitoring.* values
migration as a backward-incompatible API change.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…ng section Consolidate all observability configuration under a single monitoring: section for consistency across HyperFleet charts. - Move serviceMonitor and tracing from top-level to monitoring section - Add configmap-dashboard.yaml for Grafana sidecar provisioning - Default serviceMonitor.enabled to false (infra enables via helmfile) - Default sampler to parentbased_always_on
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@charts/README.md`:
- Line 130: Update the monitoring.serviceMonitor description in
charts/values.yaml to state that the ServiceMonitor defaults to disabled, then
regenerate charts/README.md so the documented description matches the
enabled:false default.
- Around line 129-148: The ServiceMonitor documentation has an incorrect default
description. Update the annotation for monitoring.serviceMonitor.enabled in
charts/values.yaml to state “Defaults to disabled,” then regenerate
charts/README.md so the generated description matches the false default.
🪄 Autofix
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 15c6be4a-ba8e-48a6-a98d-6199ec0a94a8
📒 Files selected for processing (12)
Makefilecharts/Chart.yamlcharts/README.mdcharts/templates/configmap-dashboard.yamlcharts/templates/deployment.yamlcharts/templates/servicemonitor.yamlcharts/values.schema.jsoncharts/values.yamldocs/configuration.mddocs/deployment.mddocs/metrics.mddocs/runbook.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/configuration.md
- charts/templates/deployment.yaml
- charts/values.schema.json
- Makefile
- docs/deployment.md
- docs/runbook.md
- charts/Chart.yaml
- docs/metrics.md
- charts/values.yaml
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/telemetry/otel_test.go (1)
19-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClear sampler variables and test the SDK default.
clearOtelEnvleavesOTEL_TRACES_SAMPLERandOTEL_TRACES_SAMPLER_ARGunchanged. Add both variables to this helper, then add a case with both variables unset to verify theparentbased_always_ondefault.Suggested change
func clearOtelEnv(t *testing.T) { t.Setenv(envOtelExporterOtlpEndpoint, "") t.Setenv(envOtelExporterOtlpTracesEndpoint, "") t.Setenv(envOtelExporterOtlpProtocol, "") t.Setenv(envOtelExporterOtlpTracesProtocol, "") + t.Setenv("OTEL_TRACES_SAMPLER", "") + t.Setenv("OTEL_TRACES_SAMPLER_ARG", "") }As per path instructions, new exported functions and critical logic paths SHOULD have tests.
🤖 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/telemetry/otel_test.go` around lines 19 - 24, Update clearOtelEnv to unset the OTEL_TRACES_SAMPLER and OTEL_TRACES_SAMPLER_ARG environment variables alongside the existing OTEL settings. Add a test case covering both sampler variables unset and verify the SDK applies the parentbased_always_on default.Source: Path instructions
🤖 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 `@pkg/telemetry/otel_test.go`:
- Around line 19-24: Update clearOtelEnv to unset the OTEL_TRACES_SAMPLER and
OTEL_TRACES_SAMPLER_ARG environment variables alongside the existing OTEL
settings. Add a test case covering both sampler variables unset and verify the
SDK applies the parentbased_always_on default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2abfb4ab-6671-485c-aa46-a9dd4c940ab7
📒 Files selected for processing (14)
Makefilecharts/Chart.yamlcharts/README.mdcharts/templates/configmap-dashboard.yamlcharts/templates/deployment.yamlcharts/templates/servicemonitor.yamlcharts/values.schema.jsoncharts/values.yamldocs/configuration.mddocs/deployment.mddocs/metrics.mddocs/runbook.mdpkg/telemetry/otel.gopkg/telemetry/otel_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (11)
- charts/templates/deployment.yaml
- docs/runbook.md
- charts/values.schema.json
- docs/configuration.md
- docs/metrics.md
- pkg/telemetry/otel.go
- charts/README.md
- docs/deployment.md
- charts/values.yaml
- charts/Chart.yaml
- Makefile
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
1 participant
Why
Part of HYPERFLEET-1363 — unify observability values under
monitoring:across all HyperFleet component charts and consolidate dashboard provisioning.Changes
serviceMonitorandtracingundermonitoring:sectionconfigmap-dashboard.yamlfor Grafana sidecar provisioning (grafana_dashboard: "1")serviceMonitor.enabledtofalse(infra enables via helmfile)parentbased_always_onvalues.schema.jsonto match new pathsmetrics.md,configuration.md,deployment.md,runbook.md) andMakefiletest commandsTest plan
make test-helmpassesOBSERVABILITY_ENABLED=true— dashboard appears in Grafana