feat(byoo-perf): add scaffolding, translation-driven rendering, and validation#422
Open
shobham-nv wants to merge 2 commits into
Open
feat(byoo-perf): add scaffolding, translation-driven rendering, and validation#422shobham-nv wants to merge 2 commits into
shobham-nv wants to merge 2 commits into
Conversation
…alidation Add the BYOO collector performance-test module under src/compute-plane-services/byoo-otel-collector/perf as a standalone Go module. - cmd/perf CLI with render/run/cleanup and dev/baseline profiles - pkg/spec: synthetic container and Helm launch specs + translate config - pkg/render: render via icms-translate function.Translate and extract the authentic collector (container sidecar and Helm utils-pod shapes) - pkg/validate: render-shape gate for placement, image, resources, ports, volumes, environment, probes, and config; fails clearly on translator drift - unit tests for spec, render, and validate The collector spec and config are produced entirely by the shared translation library so the suite exercises the same workload that ships in production. Part of #416. Closes #417. Signed-off-by: shobham <shobham@nvidia.com>
📝 WalkthroughWalkthroughThis PR adds a standalone Go performance module for BYOO OpenTelemetry collector workloads, including synthetic container and Helm specifications, translation-based rendering, shape validation, execution profiles, CLI rendering, tests, documentation, and a service version bump. ChangesBYOO performance suite
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
shobham-nv
force-pushed
the
shobham/417-byoo-perf-suite-scaffolding
branch
from
July 24, 2026 12:11
72e3ac3 to
1cb0dd0
Compare
![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)
src/compute-plane-services/byoo-otel-collector/perf/pkg/spec/spec.go (1)
56-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the remaining default images for reproducible perf runs.
DefaultCollectorImageis pinned to0.153.1, butDefaultInferenceImage,DefaultUtilsImage, andDefaultInitImagefloat on:latest. For a suite whosebaselineprofile is meant to be a "longer, repeatable run," an untaggedlatestreference can silently change between runs and skew comparisons.♻️ Pin default images
- DefaultInferenceImage = "nvcr.io/nvidia/nvcf-perf/stub-inference:latest" - DefaultUtilsImage = "nvcr.io/nvidia/nvcf-core/worker-utils:latest" - DefaultInitImage = "nvcr.io/nvidia/nvcf-core/worker-init:latest" + DefaultInferenceImage = "nvcr.io/nvidia/nvcf-perf/stub-inference:<pinned-tag>" + DefaultUtilsImage = "nvcr.io/nvidia/nvcf-core/worker-utils:<pinned-tag>" + DefaultInitImage = "nvcr.io/nvidia/nvcf-core/worker-init:<pinned-tag>"🤖 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/compute-plane-services/byoo-otel-collector/perf/pkg/spec/spec.go` around lines 56 - 58, Pin the image references assigned to DefaultInferenceImage, DefaultUtilsImage, and DefaultInitImage to explicit immutable version tags instead of :latest, matching the version-pinning approach already used by DefaultCollectorImage. Leave the existing image repositories and other defaults unchanged.
🤖 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 `@src/compute-plane-services/byoo-otel-collector/perf/pkg/render/render.go`:
- Around line 130-156: Update BenchPod to initialize the returned corev1.Pod’s
TypeMeta with apiVersion v1 and kind Pod before populating its metadata and
spec, ensuring rendered YAML/JSON is a deployable manifest while preserving the
existing pod configuration.
- Around line 80-96: The object translation loop should reject duplicate
collector-container matches instead of silently overwriting res.Collector and
res.OwnerPod. Update the *corev1.Pod handling around CollectorContainerName to
detect when found is already true, return or propagate an error for the
duplicate, and retain the existing assignment for the first match and
missing-collector behavior.
---
Nitpick comments:
In `@src/compute-plane-services/byoo-otel-collector/perf/pkg/spec/spec.go`:
- Around line 56-58: Pin the image references assigned to DefaultInferenceImage,
DefaultUtilsImage, and DefaultInitImage to explicit immutable version tags
instead of :latest, matching the version-pinning approach already used by
DefaultCollectorImage. Leave the existing image repositories and other defaults
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: a6458573-8cc3-4d98-9ac3-eee785444a90
⛔ Files ignored due to path filters (1)
src/compute-plane-services/byoo-otel-collector/perf/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
src/compute-plane-services/byoo-otel-collector/perf/README.mdsrc/compute-plane-services/byoo-otel-collector/perf/cmd/perf/main.gosrc/compute-plane-services/byoo-otel-collector/perf/go.modsrc/compute-plane-services/byoo-otel-collector/perf/pkg/profile/profile.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/render/render.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/render/render_test.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/spec/spec.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/spec/spec_test.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/validate/validate.gosrc/compute-plane-services/byoo-otel-collector/perf/pkg/validate/validate_test.go
Comment on lines
+80
to
+96
| found := false | ||
| for _, obj := range objs { | ||
| switch t := obj.(type) { | ||
| case *corev1.Pod: | ||
| for i := range t.Spec.Containers { | ||
| if t.Spec.Containers[i].Name == CollectorContainerName { | ||
| res.Collector = *t.Spec.Containers[i].DeepCopy() | ||
| res.OwnerPod = t.Name | ||
| found = true | ||
| } | ||
| } | ||
| case *corev1.Service: | ||
| if t.Name == common.ByooOTelCollectorPodNameBase { | ||
| res.Service = t.DeepCopy() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Silent overwrite if the collector container matches more than one pod.
found only guards the "missing" case; if the translator ever emitted a container named CollectorContainerName in two different pods, res.Collector/res.OwnerPod would silently end up holding whichever pod was iterated last, with no error. Given this package's explicit purpose is catching translator drift, a duplicate match should fail loudly rather than be swallowed.
🐛 Detect duplicate collector matches
found := false
for _, obj := range objs {
switch t := obj.(type) {
case *corev1.Pod:
for i := range t.Spec.Containers {
if t.Spec.Containers[i].Name == CollectorContainerName {
+ if found {
+ return nil, fmt.Errorf("found multiple %q containers in translated %s workload", CollectorContainerName, shape)
+ }
res.Collector = *t.Spec.Containers[i].DeepCopy()
res.OwnerPod = t.Name
found = true
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| found := false | |
| for _, obj := range objs { | |
| switch t := obj.(type) { | |
| case *corev1.Pod: | |
| for i := range t.Spec.Containers { | |
| if t.Spec.Containers[i].Name == CollectorContainerName { | |
| res.Collector = *t.Spec.Containers[i].DeepCopy() | |
| res.OwnerPod = t.Name | |
| found = true | |
| } | |
| } | |
| case *corev1.Service: | |
| if t.Name == common.ByooOTelCollectorPodNameBase { | |
| res.Service = t.DeepCopy() | |
| } | |
| } | |
| } | |
| found := false | |
| for _, obj := range objs { | |
| switch t := obj.(type) { | |
| case *corev1.Pod: | |
| for i := range t.Spec.Containers { | |
| if t.Spec.Containers[i].Name == CollectorContainerName { | |
| if found { | |
| return nil, fmt.Errorf("found multiple %q containers in translated %s workload", CollectorContainerName, shape) | |
| } | |
| res.Collector = *t.Spec.Containers[i].DeepCopy() | |
| res.OwnerPod = t.Name | |
| found = true | |
| } | |
| } | |
| case *corev1.Service: | |
| if t.Name == common.ByooOTelCollectorPodNameBase { | |
| res.Service = t.DeepCopy() | |
| } | |
| } | |
| } |
🤖 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/compute-plane-services/byoo-otel-collector/perf/pkg/render/render.go`
around lines 80 - 96, The object translation loop should reject duplicate
collector-container matches instead of silently overwriting res.Collector and
res.OwnerPod. Update the *corev1.Pod handling around CollectorContainerName to
detect when found is already true, return or propagate an error for the
duplicate, and retain the existing assignment for the first match and
missing-collector behavior.
Comment on lines
+130
to
+156
| func (r *Result) BenchPod(namespace string) *corev1.Pod { | ||
| pod := &corev1.Pod{} | ||
| pod.Name = r.Options.ObjectNameBase + "-collector" | ||
| pod.Namespace = namespace | ||
| pod.Labels = map[string]string{ | ||
| common.K8sAppNameLabelKey: common.ByooOTelCollectorPodNameBase, | ||
| "app.kubernetes.io/part-of": "byoo-perf", | ||
| common.BYOOMetricsEgressTargetLabelKey: common.BYOOMetricsEgressTargetLabelValue, | ||
| } | ||
|
|
||
| collector := *r.Collector.DeepCopy() | ||
| pod.Spec.Containers = []corev1.Container{collector} | ||
| pod.Spec.RestartPolicy = corev1.RestartPolicyNever | ||
|
|
||
| seen := map[string]bool{} | ||
| for _, vm := range collector.VolumeMounts { | ||
| if seen[vm.Name] { | ||
| continue | ||
| } | ||
| seen[vm.Name] = true | ||
| pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ | ||
| Name: vm.Name, | ||
| VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}, | ||
| }) | ||
| } | ||
| return pod | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
BenchPod's output is missing apiVersion/kind, so it isn't actually a deployable manifest.
pod := &corev1.Pod{} never sets TypeMeta. Since metav1.TypeMeta fields are omitempty, the YAML/JSON printed by cmd/perf render --output yaml (see main.go printObjects) will omit apiVersion: v1 / kind: Pod entirely — kubectl apply -f on this output would fail. This is already exercised by the current milestone's render --output yaml/json flag and by the README usage example, not deferred deployment work.
🐛 Set TypeMeta on the bench pod
func (r *Result) BenchPod(namespace string) *corev1.Pod {
- pod := &corev1.Pod{}
+ pod := &corev1.Pod{
+ TypeMeta: metav1.TypeMeta{
+ Kind: "Pod",
+ APIVersion: "v1",
+ },
+ }
pod.Name = r.Options.ObjectNameBase + "-collector"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (r *Result) BenchPod(namespace string) *corev1.Pod { | |
| pod := &corev1.Pod{} | |
| pod.Name = r.Options.ObjectNameBase + "-collector" | |
| pod.Namespace = namespace | |
| pod.Labels = map[string]string{ | |
| common.K8sAppNameLabelKey: common.ByooOTelCollectorPodNameBase, | |
| "app.kubernetes.io/part-of": "byoo-perf", | |
| common.BYOOMetricsEgressTargetLabelKey: common.BYOOMetricsEgressTargetLabelValue, | |
| } | |
| collector := *r.Collector.DeepCopy() | |
| pod.Spec.Containers = []corev1.Container{collector} | |
| pod.Spec.RestartPolicy = corev1.RestartPolicyNever | |
| seen := map[string]bool{} | |
| for _, vm := range collector.VolumeMounts { | |
| if seen[vm.Name] { | |
| continue | |
| } | |
| seen[vm.Name] = true | |
| pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ | |
| Name: vm.Name, | |
| VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}, | |
| }) | |
| } | |
| return pod | |
| } | |
| func (r *Result) BenchPod(namespace string) *corev1.Pod { | |
| pod := &corev1.Pod{ | |
| TypeMeta: metav1.TypeMeta{ | |
| Kind: "Pod", | |
| APIVersion: "v1", | |
| }, | |
| } | |
| pod.Name = r.Options.ObjectNameBase + "-collector" | |
| pod.Namespace = namespace | |
| pod.Labels = map[string]string{ | |
| common.K8sAppNameLabelKey: common.ByooOTelCollectorPodNameBase, | |
| "app.kubernetes.io/part-of": "byoo-perf", | |
| common.BYOOMetricsEgressTargetLabelKey: common.BYOOMetricsEgressTargetLabelValue, | |
| } | |
| collector := *r.Collector.DeepCopy() | |
| pod.Spec.Containers = []corev1.Container{collector} | |
| pod.Spec.RestartPolicy = corev1.RestartPolicyNever | |
| seen := map[string]bool{} | |
| for _, vm := range collector.VolumeMounts { | |
| if seen[vm.Name] { | |
| continue | |
| } | |
| seen[vm.Name] = true | |
| pod.Spec.Volumes = append(pod.Spec.Volumes, corev1.Volume{ | |
| Name: vm.Name, | |
| VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}, | |
| }) | |
| } | |
| return pod | |
| } |
🤖 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/compute-plane-services/byoo-otel-collector/perf/pkg/render/render.go`
around lines 130 - 156, Update BenchPod to initialize the returned corev1.Pod’s
TypeMeta with apiVersion v1 and kind Pod before populating its metadata and
spec, ensuring rendered YAML/JSON is a deployable manifest while preserving the
existing pod configuration.
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 `@src/compute-plane-services/byoo-otel-collector/VERSION`:
- Line 1: Update the VERSION value for the byoo-otel-collector to match the
collector configuration’s pinned v0.157.0 release, ensuring the VERSION file and
otel-collector-build.yaml use consistent release metadata.
🪄 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: 0be78c38-5067-4968-a821-856c75aba73a
📒 Files selected for processing (1)
src/compute-plane-services/byoo-otel-collector/VERSION
| @@ -1 +1 @@ | |||
| 0.157.0 | |||
| 0.158.0 | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="src/compute-plane-services/byoo-otel-collector"
rg -n --hidden --glob '!**/.git/**' \
'0\.157\.0|0\.158\.0|collector.*version|VERSION' "$root"Repository: NVIDIA/nvcf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
root="src/compute-plane-services/byoo-otel-collector"
printf '\n== VERSION ==\n'
cat -n "$root/VERSION"
printf '\n== otel-collector-build.yaml version lines ==\n'
rg -n '0\.157\.0|0\.158\.0|major_minor|version' "$root/otel-collector-build.yaml" "$root/AGENTS.md" "$root/scripts/update-collector-version.sh" | sed -n '1,120p'
printf '\n== relevant release wiring ==\n'
sed -n '50,60p' "$root/AGENTS.md"
sed -n '1,40p' "$root/otel-collector-build.yaml"Repository: NVIDIA/nvcf
Length of output: 14758
Align VERSION with the collector config
src/compute-plane-services/byoo-otel-collector/VERSION is 0.158.0, but src/compute-plane-services/byoo-otel-collector/otel-collector-build.yaml still pins v0.157.0 throughout. This drifts the release version from the collector major/minor and will produce inconsistent release metadata.
🤖 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/compute-plane-services/byoo-otel-collector/VERSION` at line 1, Update the
VERSION value for the byoo-otel-collector to match the collector configuration’s
pinned v0.157.0 release, ensuring the VERSION file and otel-collector-build.yaml
use consistent release metadata.
Source: Coding guidelines
mikeyrcamp
reviewed
Jul 24, 2026
| GOWORK=off go run ./cmd/perf render --shape both | ||
|
|
||
| # Render one shape and print the deployable workload as YAML. | ||
| GOWORK=off go run ./cmd/perf render --shape container --output yaml |
Contributor
There was a problem hiding this comment.
Will this run it using kubectl against a cluster?
mikeyrcamp
reviewed
Jul 24, 2026
| package main | ||
|
|
||
| import ( | ||
| "flag" |
Contributor
There was a problem hiding this comment.
This CLI is fairly comprehensive with many of sub-commands. I would recommend using cobra as we were are with other CLIs
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.
Add the BYOO collector performance-test module under
src/compute-plane-services/byoo-otel-collector/perfas a standalone Go module.cmd/perfCLI with render/run/cleanup and dev/baseline profilespkg/spec: synthetic container and Helm launch specs + translate configpkg/render: render viaicms-translatefunction.Translateand extract the authentic collector (container sidecar and Helm utils-pod shapes)pkg/validate: render-shape gate for placement, image, resources, ports, volumes, environment, probes, and config; fails clearly on translator driftThe collector spec and config are produced entirely by the shared translation library so the suite exercises the same workload that ships in production.
TL;DR
Adds the scaffolding for a BYOO OpenTelemetry collector performance-test suite. This first slice renders the collector workload directly through the shared
icms-translatelibrary (the same code NVCA uses in production), extracts the authentic collector container, and validates its shape. Load generation and measurement land in follow-up PRs.Additional Details
GOWORK=off) with areplacepointing at the localicms-translatelibrary, so it always renders exactly the workload NVCA produces rather than a hand-maintained copy.pkg/rendercallsfunction.Translatefor both the container-function shape (collector as a sidecar) and the Helm-function shape (collector on the utils pod with a ClusterIP Service), then extracts the real collector container plus its owning pod/service.BenchPodwraps that container with lightweightemptyDirstand-ins for co-located containers so it is deployable on k3d.pkg/validateacts as a drift gate: if the translation library changes the collector's placement, image, resources, ports, volume mounts, env, probes, or config args, validation fails with a clear message before any load is run.For the Reviewer
pkg/render/render.go(translation + extraction),pkg/validate/validate.go(drift gate), andpkg/spec/spec.go(synthetic launch specs).pkg/specmatch how NVCA constructs container and Helm function messages.For QA
From
src/compute-plane-services/byoo-otel-collector/perf:GOWORK=off go build ./... GOWORK=off go vet ./... GOWORK=off go test ./... GOWORK=off go run ./cmd/perf render --shape=container,helmQA Needed? No — unit-test-level change, no runtime behavior shipped to production.
Issues
Part of #416. Closes #417.
Checklist
Summary by CodeRabbit