feat(nvca): wire BYOO metric subset config#326
Merged
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:
📝 WalkthroughWalkthroughNVCA and the BYOO OTel collector add metric subset configuration, replace SRE metrics naming and pipeline wiring, document mergeConfig examples, and propagate combined collector environment variables through miniservice, function, task, and webhook paths. ChangesBYOO metric subset configuration and collector pipeline
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MergeConfig
participant AgentConfig
participant MiniserviceReconciler
participant ContainerExecution
participant BYOOOTelCollector
MergeConfig->>AgentConfig: decode byooMetricSubset settings
AgentConfig->>MiniserviceReconciler: provide BYOO OTel collector env vars
AgentConfig->>ContainerExecution: provide BYOO OTel collector env vars
MiniserviceReconciler->>BYOOOTelCollector: inject collector env vars
ContainerExecution->>BYOOOTelCollector: inject collector env vars
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-feat-nvca-byoo-sre-values.docs.buildwithfern.com/nvcf |
![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
🤖 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 `@docs/ngc-managed/cluster-management/configuration.md`:
- Line 736: Update the “BYOO SRE metrics example” documentation heading to plain
text by removing the Markdown bold markers, while preserving the heading text
and colon.
🪄 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: 65d1f8cf-a1e7-4000-8d4a-bfa988447a45
⛔ Files ignored due to path filters (3)
src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config/types.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config/zz_generated.deepcopy.gois excluded by!**/vendor/**,!**/zz_generated.*src/libraries/go/lib/pkg/types/nvca/config/zz_generated.deepcopy.gois excluded by!**/zz_generated.*
📒 Files selected for processing (16)
deploy/helm/nvca-operator/nvca-operator/values.yamldocs/ngc-managed/cluster-management/configuration.mddocs/user/cluster-management/configuration.mdsrc/compute-plane-services/nvca/internal/miniservice/reconcile.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_test.gosrc/compute-plane-services/nvca/internal/util/k8sutil/byoo_log_chunking.gosrc/compute-plane-services/nvca/internal/util/k8sutil/byoo_log_chunking_test.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend.gosrc/compute-plane-services/nvca/pkg/nvca/k8scomputebackend_task_container.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile_test.gosrc/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook.gosrc/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook_test.gosrc/libraries/go/lib/pkg/types/nvca/config/config_test.gosrc/libraries/go/lib/pkg/types/nvca/config/deepcopy_test.gosrc/libraries/go/lib/pkg/types/nvca/config/types.gosrc/libraries/go/lib/pkg/types/nvca/config/types_test.go
4 tasks
Add NVCA agent config for BYOO SRE metrics collector settings and pass the resulting environment variables through the existing BYOO OTel collector-only injection path. This lets Helm users configure the SRE metrics pipeline through agentConfig.mergeConfig without adding dedicated chart values. Document the mergeConfig shape and extend focused tests for config decoding, env rendering, operator config merging, and collector-only pod mutation. Closes #325 Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
kristinapathak
force-pushed
the
feat/nvca-byoo-sre-values
branch
from
July 21, 2026 21:47
2ffb514 to
2d935f8
Compare
estroz
requested changes
Jul 21, 2026
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
kristinapathak
changed the title
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 `@docs/user/cluster-management/configuration.md`:
- Around line 865-870: Update the filterConfig example’s metric_conditions
predicate from equality to inequality so BpsInstrument is selected as the
exported subset, matching the default filter behavior; leave the surrounding
configuration unchanged.
🪄 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: aae2a577-b9b7-46a6-9f56-6df92a8baac4
⛔ Files ignored due to path filters (3)
src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config/types.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config/zz_generated.deepcopy.gois excluded by!**/vendor/**,!**/zz_generated.*src/libraries/go/lib/pkg/types/nvca/config/zz_generated.deepcopy.gois excluded by!**/zz_generated.*
📒 Files selected for processing (18)
deploy/helm/nvca-operator/nvca-operator/values.yamldocs/ngc-managed/cluster-management/configuration.mddocs/user/cluster-management/configuration.mdsrc/compute-plane-services/byoo-otel-collector/README.mdsrc/compute-plane-services/byoo-otel-collector/examples/otelconfigs/k8s/config_function_container_metric_subset.yamlsrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/embed_config.gosrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/otelconfig.gosrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/otelconfig_test.gosrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/render.gosrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/render_test.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_test.gosrc/compute-plane-services/nvca/internal/util/k8sutil/byoo_env_vars_test.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile_test.gosrc/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook_test.gosrc/libraries/go/lib/pkg/types/nvca/config/config_test.gosrc/libraries/go/lib/pkg/types/nvca/config/deepcopy_test.gosrc/libraries/go/lib/pkg/types/nvca/config/types.gosrc/libraries/go/lib/pkg/types/nvca/config/types_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- src/compute-plane-services/nvca/internal/util/k8sutil/byoo_env_vars_test.go
- src/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile_test.go
- src/compute-plane-services/nvca/pkg/webhook/miniservice_mutating_webhook_test.go
estroz
approved these changes
Jul 21, 2026
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/libraries/go/lib/pkg/types/nvca/config/deepcopy_test.go (1)
72-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that
DeepCopyreturns a distinct object.This test checks field values but would pass if
DeepCopy()returnedorig. Add the sameassert.NotSamecheck used by the neighboring deep-copy test.Suggested assertion
out := orig.DeepCopy() require.NotNil(t, out) + assert.NotSame(t, orig, out) assert.Equal(t, orig.Enabled, out.Enabled)🤖 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 `@src/libraries/go/lib/pkg/types/nvca/config/deepcopy_test.go` around lines 72 - 81, Update TestDeepCopy_BYOOMetricSubsetConfig to assert that the result of orig.DeepCopy() is a distinct object from orig, matching the assert.NotSame check in the neighboring deep-copy test while preserving the existing field-value assertions.
🤖 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 `@src/libraries/go/lib/pkg/types/nvca/config/deepcopy_test.go`:
- Around line 72-81: Update TestDeepCopy_BYOOMetricSubsetConfig to assert that
the result of orig.DeepCopy() is a distinct object from orig, matching the
assert.NotSame check in the neighboring deep-copy test while preserving the
existing field-value assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 20e30ffb-98a0-4cfe-9782-adc89de4d783
⛔ Files ignored due to path filters (3)
src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config/types.gois excluded by!**/vendor/**src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config/zz_generated.deepcopy.gois excluded by!**/vendor/**,!**/zz_generated.*src/libraries/go/lib/pkg/types/nvca/config/zz_generated.deepcopy.gois excluded by!**/zz_generated.*
📒 Files selected for processing (16)
deploy/helm/nvca-operator/nvca-operator/values.yamldocs/ngc-managed/cluster-management/configuration.mddocs/user/cluster-management/configuration.mdsrc/compute-plane-services/byoo-otel-collector/README.mdsrc/compute-plane-services/byoo-otel-collector/examples/otelconfigs/k8s/config_function_container_metric_subset.yamlsrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/embed_config.gosrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/otelconfig.gosrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/otelconfig_test.gosrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/render.gosrc/compute-plane-services/byoo-otel-collector/internal/otelconfig/render_test.gosrc/compute-plane-services/nvca/internal/miniservice/reconcile_test.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile_test.gosrc/libraries/go/lib/pkg/types/nvca/config/config_test.gosrc/libraries/go/lib/pkg/types/nvca/config/deepcopy_test.gosrc/libraries/go/lib/pkg/types/nvca/config/types.gosrc/libraries/go/lib/pkg/types/nvca/config/types_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- src/libraries/go/lib/pkg/types/nvca/config/config_test.go
- src/libraries/go/lib/pkg/types/nvca/config/types_test.go
- deploy/helm/nvca-operator/nvca-operator/values.yaml
- src/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile_test.go
- docs/ngc-managed/cluster-management/configuration.md
- src/compute-plane-services/byoo-otel-collector/internal/otelconfig/otelconfig_test.go
- src/compute-plane-services/byoo-otel-collector/README.md
- src/compute-plane-services/byoo-otel-collector/internal/otelconfig/otelconfig.go
- src/compute-plane-services/nvca/internal/miniservice/reconcile_test.go
- src/compute-plane-services/byoo-otel-collector/examples/otelconfigs/k8s/config_function_container_metric_subset.yaml
- src/compute-plane-services/byoo-otel-collector/internal/otelconfig/render.go
- src/compute-plane-services/byoo-otel-collector/internal/otelconfig/render_test.go
- docs/user/cluster-management/configuration.md
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
kristinapathak
enabled auto-merge
July 21, 2026 23:07
Contributor
|
🎉 This PR is included in version helm-nvca-operator-v1.16.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
3 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.
TL;DR
agentConfig.mergeConfig.Additional Details
byooMetricSubset.byooWorkloadMetricsDropLabelsfor drops on the generated workloadmetricspipeline.BYOO_METRIC_SUBSET_ENABLED,BYOO_METRIC_SUBSET_FILTER_CONFIG, andBYOO_WORKLOAD_METRICS_DROP_LABELS.metrics/metric_subset,filter/metric_subset, andbatch/metric_subset.metric_subset_enabledas the default workload drop label only when metric subset is enabled.For the Reviewer
src/libraries/go/lib/pkg/types/nvca/config/types.goand the BYOO collector render path.Testing and Documentation
go test ./internal/otelconfig -count=1go test ./pkg/types/nvca/config -count=1helm templatefor default chart values and abyooMetricSubset/byooWorkloadMetricsDropLabelsmergeConfig overlay.Issues
Closes #325
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation