RHOAIENG-25586 | feat: Enable 3rd party traces export #2226
Conversation
WalkthroughAdds an optional Traces.Exporters map to APIs/CRDs; updates deepcopy to deep-copy exporters; validates and normalizes exporter configs in the controller; exposes per-exporter YAML into the OpenTelemetry Collector template; adds template indent helpers and registers them; extends e2e tests for exporter behavior and reserved-name validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CRD
participant Controller
participant TemplateEngine
participant OTelCollector
User->>CRD: Create/Update Monitoring or DSCInitialization with traces.exporters
CRD->>Controller: Reconcile Monitoring resources
Controller->>Controller: Validate names (regex + reserved) and normalize exporter configs (RawExtension → YAML)
Controller->>TemplateEngine: Provide template data including .TracesExporters
TemplateEngine->>OTelCollector: Rendered YAML (per-exporter top-level blocks + pipeline exporters) deployed
OTelCollector-->>User: Collector configured with exporters
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/16598558656 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/api-overview.md (1)
3023-3026: Clarify exporter-name constraints & add minimal YAML exampleThe implementation rejects “reserved” exporter names, yet the doc does not list what names are reserved or link to that validation logic. Readers will have no way to understand why their exporter is rejected.
Also, a short YAML snippet would greatly improve usability:
traces: exporters: otlp/custom: endpoint: "https://tempo.example.com" compression: gzipConsider appending a sentence such as:
“Exporter names must not clash with the predefined collectors
otlp,jaeger, orzipkin(see monitoring controller validation).”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/services/v1alpha1/monitoring_types.go(2 hunks)api/services/v1alpha1/zz_generated.deepcopy.go(3 hunks)config/crd/bases/dscinitialization.opendatahub.io_dscinitializations.yaml(1 hunks)config/crd/bases/services.platform.opendatahub.io_monitorings.yaml(1 hunks)docs/api-overview.md(1 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(2 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(2 hunks)internal/controller/services/monitoring/resources/opentelemetry-collector.tmpl.yaml(2 hunks)tests/e2e/monitoring_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/e2e/monitoring_test.go
- api/services/v1alpha1/monitoring_types.go
- internal/controller/services/monitoring/monitoring_controller_actions.go
- internal/controller/services/monitoring/monitoring_controller_support.go
- config/crd/bases/dscinitialization.opendatahub.io_dscinitializations.yaml
- api/services/v1alpha1/zz_generated.deepcopy.go
- internal/controller/services/monitoring/resources/opentelemetry-collector.tmpl.yaml
- config/crd/bases/services.platform.opendatahub.io_monitorings.yaml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mlassak
PR: opendatahub-io/opendatahub-operator#2010
File: config/crd/kustomization.yaml:22-22
Timestamp: 2025-07-22T10:32:09.737Z
Learning: In the opendatahub-operator repository, when FeatureTrackers are being removed or deprecated, the FeatureTracker CRD reference in config/crd/kustomization.yaml should be kept for backward compatibility during the migration period, even if some components no longer use FeatureTrackers.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run tests and collect coverage on tests/integration
- GitHub Check: Run tests and collect coverage on internal and pkg
|
/test opendatahub-operator-e2e |
1 similar comment
|
/test opendatahub-operator-e2e |
|
/cc @MarianMacik |
internal/controller/services/monitoring/monitoring_controller_actions.go
Outdated
Show resolved
Hide resolved
den-rgb
force-pushed
the
RHOAIENG-25586
branch
3 times, most recently
from
bf7ae2d to
b9b0184
Compare
| @@ -175,7 +175,7 @@ func deployOpenTelemetryCollector(ctx context.Context, rr *odhtypes.Reconciliati | |||
| status.ConditionOpenTelemetryCollectorAvailable, | |||
| ) | |||
|
|
|||
| if monitoring.Spec.Metrics != nil { | |||
| if mon.Spec.Metrics != nil || mon.Spec.Traces != nil { | |||
There was a problem hiding this comment.
if this is a bug fix, should be stated in the PR description.
preferably create a PR for the fix
There was a problem hiding this comment.
not a bug fix, wasn't required prior to the exporters being added
| exporters: [otlp/tempo{{ if .TracesExporters }}{{ range $name, $config := .TracesExporters }}, {{ $name }}{{ end }}{{ end }}] | ||
| {{ end }} | ||
| {{ if .Metrics }} |
There was a problem hiding this comment.
can consider use indent from sprig then you no need handle replace and space etc in the function https://github.com/Masterminds/sprig/blob/989da45d7c082c6ec85cf95f59e23b461cdafe03/docs/strings.md#indent
something like
{{- if .TracesExporters }}
{{- range $name, $yamlConfig := .TracesExporters }}
{{$name}}: |
{{$yamlConfig | indent 12}}
{{- end }}
{{- end }}
internal/controller/services/monitoring/monitoring_controller_support.go
Outdated
Show resolved
Hide resolved
|
/test opendatahub-operator-e2e |
1 similar comment
|
/test opendatahub-operator-e2e |
den-rgb
force-pushed
the
RHOAIENG-25586
branch
2 times, most recently
from
d819151 to
0b01b25
Compare
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
den-rgb
force-pushed
the
RHOAIENG-25586
branch
2 times, most recently
from
cb804f0 to
0d4241e
Compare
den-rgb
force-pushed
the
RHOAIENG-25586
branch
from
2f41b8f to
5da021a
Compare
|
/test-integration |
|
ODH Operator Integration Tests #14 triggered by #2226 (comment) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/e2e/monitoring_test.go (3)
209-212: Use null instead of {} for metrics to avoid relying on controller canonicalizationSet
.spec.monitoring.metricsto null for clarity and to match other tests. This was raised previously and remains applicable.Apply:
- WithMutateFunc(testf.TransformPipeline( - testf.Transform(`.spec.monitoring.metrics = %s`, `{}`), - testf.Transform(`.spec.monitoring.traces = null`), - )), + WithMutateFunc(testf.TransformPipeline( + testf.Transform(`.spec.monitoring.metrics = null`), + testf.Transform(`.spec.monitoring.traces = null`), + )),
275-312: Strengthen assertions to verify exporter configs are mappings with expected fieldsThe current checks only assert presence of exporter keys and pipeline references. Add field-level assertions to catch cases where exporters are rendered as block scalars or otherwise malformed. This mirrors a prior suggestion.
Apply:
tc.EnsureResourceExists( WithMinimalObject(gvk.OpenTelemetryCollector, types.NamespacedName{Name: "data-science-collector", Namespace: dsci.Spec.Monitoring.Namespace}), WithCondition(jq.Match(`.spec.config.exporters | has("jaeger")`)), WithCondition(jq.Match(`.spec.config.exporters | has("otlp/custom")`)), WithCondition(jq.Match(`.spec.config.exporters | has("otlp/tempo")`)), // Default tempo exporter should still exist + // Validate nested fields to ensure exporters are proper mappings, not strings + WithCondition(jq.Match(`.spec.config.exporters.jaeger.endpoint == "%s"`, "http://jaeger-collector:14250")), + WithCondition(jq.Match(`.spec.config.exporters."otlp/custom".endpoint == "%s"`, "http://custom-endpoint:4317")), + // Optional: validate TLS/headers shape as well + WithCondition(jq.Match(`.spec.config.exporters.jaeger.tls.insecure == %t`, true)), + WithCondition(jq.Match(`.spec.config.exporters."otlp/custom".headers."api-key" == "%s"`, "secret-key")), WithCondition(jq.Match(`.spec.config.service.pipelines.traces.exporters | contains(["jaeger"])`)), WithCondition(jq.Match(`.spec.config.service.pipelines.traces.exporters | contains(["otlp/custom"])`)), WithCondition(jq.Match(`.spec.config.service.pipelines.traces.exporters | contains(["otlp/tempo"])`)), )
338-340: Fix compile error: unused parameter in getTempoMonolithicNameThe dsci parameter isn’t used; Go forbids unused parameters. Rename to underscore to indicate intentional ignore.
Apply:
-func getTempoMonolithicName(dsci *dsciv1.DSCInitialization) string { +func getTempoMonolithicName(_ *dsciv1.DSCInitialization) string { return "data-science-tempomonolithic" }
🧹 Nitpick comments (5)
tests/e2e/monitoring_test.go (2)
314-336: Optional: assert the collector wasn’t polluted by the reserved-name configYou already assert the Monitoring condition/message. Consider adding a sanity check that the OpenTelemetryCollector did not pick up a user-defined “otlp/tempo” exporter override.
Example addition:
tc.EnsureResourceExists( WithMinimalObject(gvk.Monitoring, types.NamespacedName{Name: "default-monitoring"}), WithCondition(jq.Match(`.status.conditions[] | select(.type == "%s") | .status == "%s"`, status.ConditionTypeProvisioningSucceeded, metav1.ConditionFalse)), WithCondition(jq.Match(`.status.conditions[] | select(.type == "%s") | .message | contains("reserved")`, status.ConditionTypeProvisioningSucceeded)), ) + +// Ensure the collector still only has the default otlp/tempo exporter (no override under exporters from user config) +tc.EnsureResourceExists( + WithMinimalObject(gvk.OpenTelemetryCollector, types.NamespacedName{Name: "data-science-collector", Namespace: dsci.Spec.Monitoring.Namespace}), + WithCondition(jq.Match(`.spec.config.exporters | has("otlp/tempo")`)), +)
371-372: Nit: avoid setting exporters to an empty map when not neededYou can omit the field entirely; the controller already initializes an empty map for template rendering. Keeping spec lean reduces noise.
- "exporters": map[string]interface{}{}, + // omit exporters when not needed; controller handles empty map defaultsinternal/controller/services/monitoring/monitoring_controller_support.go (3)
32-40: Reserve names helper is fine; consider avoiding per-call map allocationTiny improvement: move the reserved set to a package-level var to avoid allocating the map each call.
Apply within this function:
-func isReservedName(n string) bool { - reservedNames := map[string]bool{ - "otlp/tempo": true, - "prometheus": true, - } - return reservedNames[n] -} +func isReservedName(n string) bool { + return reservedExporterNames[n] +}Add outside (top-level):
var reservedExporterNames = map[string]bool{ "otlp/tempo": true, "prometheus": true, }
42-104: Prefer resource.Quantity.IsZero() over string comparisonsRelying on .String() == "0" is brittle. Use IsZero() for robustness across quantity representations.
Example changes:
- cpuLimit := metrics.Resources.CPULimit.String() - if cpuLimit == "" || cpuLimit == "0" { + cpuLimit := metrics.Resources.CPULimit.String() + if metrics.Resources.CPULimit.IsZero() || cpuLimit == "" { cpuLimit = "500m" } ... - memoryLimit := metrics.Resources.MemoryLimit.String() - if memoryLimit == "" || memoryLimit == "0" { + memoryLimit := metrics.Resources.MemoryLimit.String() + if metrics.Resources.MemoryLimit.IsZero() || memoryLimit == "" { memoryLimit = "512Mi" } ... - cpuRequest := metrics.Resources.CPURequest.String() - if cpuRequest == "" || cpuRequest == "0" { + cpuRequest := metrics.Resources.CPURequest.String() + if metrics.Resources.CPURequest.IsZero() || cpuRequest == "" { cpuRequest = "100m" } ... - memoryRequest := metrics.Resources.MemoryRequest.String() - if memoryRequest == "" || memoryRequest == "0" { + memoryRequest := metrics.Resources.MemoryRequest.String() + if metrics.Resources.MemoryRequest.IsZero() || memoryRequest == "" { memoryRequest = "256Mi" }
105-152: Render structured exporter objects instead of YAML strings for template safetyRound-tripping to YAML strings forces templates to handle indented block scalars, which is error-prone. Pass the parsed map directly and render via toYaml/indent in the template. Also consider emitting a deterministic names slice for stable ordering.
Apply:
-func validateExporters(exporters map[string]runtime.RawExtension) (map[string]string, error) { - validatedExporters := make(map[string]string) +func validateExporters(exporters map[string]runtime.RawExtension) (map[string]any, error) { + validatedExporters := make(map[string]any) @@ - var config map[string]interface{} + var config map[string]any if err := yaml.Unmarshal(raw, &config); err != nil { return nil, fmt.Errorf("failed to unmarshal exporter config for '%s': %w", name, err) } - - // Convert config back to YAML string for template rendering - configYAML, err := yaml.Marshal(config) - if err != nil { - return nil, fmt.Errorf("failed to marshal exporter config for '%s': %w", name, err) - } - // Store the YAML string for template rendering with the indent template function - validatedExporters[name] = strings.TrimSpace(string(configYAML)) + // Store structured config for template rendering + validatedExporters[name] = config } return validatedExporters, nil }Then in addTracesTemplateData:
- validatedExporters := make(map[string]string) + validatedExporters := make(map[string]any)Template follow-up (outside this file):
- Iterate over sorted names (optional) and render each exporter with: toYaml (index .TracesExporters $name) | indent N
If you want deterministic order, additionally collect/sort names and pass TracesExporterNames into template data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
api/services/v1alpha1/monitoring_types.go(2 hunks)api/services/v1alpha1/zz_generated.deepcopy.go(3 hunks)bundle/manifests/dscinitialization.opendatahub.io_dscinitializations.yaml(1 hunks)bundle/manifests/opendatahub-operator.clusterserviceversion.yaml(2 hunks)bundle/manifests/services.platform.opendatahub.io_monitorings.yaml(1 hunks)config/crd/bases/dscinitialization.opendatahub.io_dscinitializations.yaml(1 hunks)config/crd/bases/services.platform.opendatahub.io_monitorings.yaml(1 hunks)docs/api-overview.md(1 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(1 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(3 hunks)internal/controller/services/monitoring/resources/opentelemetry-collector.tmpl.yaml(2 hunks)pkg/controller/actions/render/template/action_render_templates.go(2 hunks)pkg/feature/manifest/types.go(2 hunks)pkg/utils/template/template.go(1 hunks)tests/e2e/monitoring_test.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- internal/controller/services/monitoring/monitoring_controller_actions.go
- pkg/feature/manifest/types.go
- api/services/v1alpha1/monitoring_types.go
- docs/api-overview.md
- bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
- api/services/v1alpha1/zz_generated.deepcopy.go
- pkg/controller/actions/render/template/action_render_templates.go
- bundle/manifests/dscinitialization.opendatahub.io_dscinitializations.yaml
- config/crd/bases/dscinitialization.opendatahub.io_dscinitializations.yaml
- pkg/utils/template/template.go
- config/crd/bases/services.platform.opendatahub.io_monitorings.yaml
- internal/controller/services/monitoring/resources/opentelemetry-collector.tmpl.yaml
- bundle/manifests/services.platform.opendatahub.io_monitorings.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T07:56:13.158Z
Learnt from: GowthamShanmugam
PR: opendatahub-io/opendatahub-operator#2159
File: tests/e2e/creation_test.go:486-490
Timestamp: 2025-08-08T07:56:13.158Z
Learning: In tests/e2e/creation_test.go, ValidateComponentsDeploymentFailure intentionally uses an extremely restrictive ResourceQuota to simulate component deployment failures; CPU quantities must use valid forms (e.g., "1m" or "0"), since "0.1m" is invalid and causes resource.MustParse to panic.
Applied to files:
tests/e2e/monitoring_test.go
🧬 Code Graph Analysis (2)
internal/controller/services/monitoring/monitoring_controller_support.go (2)
api/services/v1alpha1/monitoring_types.go (2)
Metrics(46-51)Traces(87-99)pkg/cluster/gvk/gvk.go (2)
Secret(132-136)Namespace(24-28)
tests/e2e/monitoring_test.go (4)
tests/e2e/resource_options_test.go (3)
WithMutateFunc(194-198)WithMinimalObject(140-155)WithCondition(203-207)pkg/utils/test/testf/testf_support.go (2)
TransformPipeline(50-61)Transform(78-106)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)internal/controller/status/status.go (1)
ConditionTypeProvisioningSucceeded(95-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: golangci-lint
- GitHub Check: Run tests and collect coverage on tests/integration
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (7)
tests/e2e/monitoring_test.go (5)
15-15: Import alias for dsciv1 looks correctThis aligns with the new helper signature usage below.
49-49: Good addition: explicit default traces content test earlier in the suitePlacing this earlier helps ensure the base state before later mutations.
60-62: Nice coverage for custom exporters and reserved-name validationThese two cases exercise both happy-path and validation error scenarios for the new exporters field.
153-154: Retention expectation update to 90d is consistentMatches the new default behavior asserted elsewhere.
426-426: Call site updated to pass dsciMatches the updated helper signature (after marking parameter unused as suggested).
internal/controller/services/monitoring/monitoring_controller_support.go (2)
5-15: Imports look appropriate for RawExtension handling and YAML normalizationjson for RawExtension.Object fallback, yaml.v3 for map parsing, and runtime for RawExtension are all justified.
154-186: Traces template data looks coherent; endpoints/backends/retention wiring LGTMUsing namespace-scoped endpoints and populating retention across backends aligns with the tests. No issues found.
den-rgb
force-pushed
the
RHOAIENG-25586
branch
from
5da021a to
30dcf35
Compare
|
/test-integration |
|
ODH Operator Integration Tests #15 triggered by #2226 (comment) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/e2e/monitoring_test.go (3)
209-212: Prefer null over {} for metrics to avoid relying on controller canonicalizationSetting
.spec.monitoring.metrics = {}assumes the controller will normalize to null. Set it to null directly for clarity and stability.Apply this diff:
tc.EventuallyResourceCreatedOrUpdated( WithMinimalObject(gvk.DSCInitialization, tc.DSCInitializationNamespacedName), WithMutateFunc(testf.TransformPipeline( - testf.Transform(`.spec.monitoring.metrics = %s`, `{}`), + testf.Transform(`.spec.monitoring.metrics = null`), testf.Transform(`.spec.monitoring.traces = null`), )), )
275-312: Strengthen assertions to ensure exporter configs are mappings, not block-scalarsCurrently we only assert presence of keys and pipeline membership. Add checks for nested fields (endpoint, tls, headers) so a mis-rendered YAML block-scalar won’t pass.
Apply this diff to enhance the test:
tc.EnsureResourceExists( WithMinimalObject(gvk.OpenTelemetryCollector, types.NamespacedName{Name: "data-science-collector", Namespace: dsci.Spec.Monitoring.Namespace}), WithCondition(jq.Match(`.spec.config.exporters | has("jaeger")`)), WithCondition(jq.Match(`.spec.config.exporters | has("otlp/custom")`)), WithCondition(jq.Match(`.spec.config.exporters | has("otlp/tempo")`)), // Default tempo exporter should still exist + WithCondition(jq.Match(`.spec.config.exporters.jaeger.endpoint == "%s"`, "http://jaeger-collector:14250")), + WithCondition(jq.Match(`.spec.config.exporters.jaeger.tls.insecure == %t`, true)), + WithCondition(jq.Match(`.spec.config.exporters."otlp/custom".endpoint == "%s"`, "http://custom-endpoint:4317")), + WithCondition(jq.Match(`.spec.config.exporters."otlp/custom".headers."api-key" == "%s"`, "secret-key")), WithCondition(jq.Match(`.spec.config.service.pipelines.traces.exporters | contains(["jaeger"])`)), WithCondition(jq.Match(`.spec.config.service.pipelines.traces.exporters | contains(["otlp/custom"])`)), WithCondition(jq.Match(`.spec.config.service.pipelines.traces.exporters | contains(["otlp/tempo"])`)), )This will catch cases where the template injects exporter definitions as strings via a block scalar.
338-340: Fix compile error: unused parameter in getTempoMonolithicNameThe dsci parameter is not used, which will cause a Go compile error. Mark it intentionally unused.
Apply this diff:
-func getTempoMonolithicName(dsci *dsciv1.DSCInitialization) string { +func getTempoMonolithicName(_ *dsciv1.DSCInitialization) string { return "data-science-tempomonolithic" }
🧹 Nitpick comments (3)
bundle/manifests/services.platform.opendatahub.io_monitorings.yaml (1)
143-151: Good addition: flexible exporters map with preserved unknown fieldsAllowing arbitrary exporter configs via additionalProperties + preserve-unknown-fields is the right choice for RawExtension-backed data.
Consider mentioning reserved names in the description to set user expectations upfront (the controller rejects them). For example: “Note: Some exporter names are reserved (e.g., otlp/tempo, prometheus) and will be rejected by the controller.”
internal/controller/services/monitoring/monitoring_controller_support.go (2)
105-152: Avoid passing exporter configs as YAML strings; pass structured maps insteadRe-marshal to YAML and storing strings encourages rendering via block scalars, which risks invalid collector config (exporters become strings instead of mappings). Prefer returning structured maps to the template and render via toYaml + indent (or your indent helper).
Apply this diff within validateExporters:
-func validateExporters(exporters map[string]runtime.RawExtension) (map[string]string, error) { - validatedExporters := make(map[string]string) +func validateExporters(exporters map[string]runtime.RawExtension) (map[string]any, error) { + validatedExporters := make(map[string]any) @@ - // Convert RawExtension to a map for validation and YAML conversion - var config map[string]interface{} + // Convert RawExtension to a map for validation + var config map[string]any if err := yaml.Unmarshal(raw, &config); err != nil { return nil, fmt.Errorf("failed to unmarshal exporter config for '%s': %w", name, err) } - // Convert config back to YAML string for template rendering - configYAML, err := yaml.Marshal(config) - if err != nil { - return nil, fmt.Errorf("failed to marshal exporter config for '%s': %w", name, err) - } - // Store the YAML string for template rendering with the indent template function - validatedExporters[name] = strings.TrimSpace(string(configYAML)) + // Store the structured config for template rendering as a mapping + validatedExporters[name] = config } return validatedExporters, nil }And in addTracesTemplateData:
- // Always initialize validatedExporters to avoid template rendering failures - validatedExporters := make(map[string]string) + // Always initialize validatedExporters to avoid template rendering failures + validatedExporters := make(map[string]any) if traces.Exporters != nil { var err error validatedExporters, err = validateExporters(traces.Exporters) if err != nil { return err } } // Always set TracesExporters, even if empty, to prevent template rendering failures templateData["TracesExporters"] = validatedExportersAdditionally, remove the now-unused import:
import ( "context" "encoding/json" "errors" "fmt" "regexp" "strconv" - "strings"Update the OpenTelemetry Collector template to render each exporter as a mapping, e.g.:
- exporters:
- {{ range $name, $_ := .TracesExporters }}
- {{ $name }}:
- {{ toYaml (index $.TracesExporters $name) | indent 6 }}
- {{ end }}
If you prefer deterministic ordering, collect and sort the names in the controller and iterate over that slice in the template.
154-186: Traces template data looks correct; ensure the template consumes TracesExporters as mappingsBackend/retention/endpoint values are set as expected. If you keep TracesExporters as structured maps (see prior comment), verify the template uses toYaml + indent (or equivalent) to avoid block scalars.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
api/services/v1alpha1/monitoring_types.go(2 hunks)api/services/v1alpha1/zz_generated.deepcopy.go(3 hunks)bundle/manifests/dscinitialization.opendatahub.io_dscinitializations.yaml(1 hunks)bundle/manifests/opendatahub-operator.clusterserviceversion.yaml(1 hunks)bundle/manifests/services.platform.opendatahub.io_monitorings.yaml(1 hunks)config/crd/bases/dscinitialization.opendatahub.io_dscinitializations.yaml(1 hunks)config/crd/bases/services.platform.opendatahub.io_monitorings.yaml(1 hunks)docs/api-overview.md(1 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(1 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(3 hunks)internal/controller/services/monitoring/resources/opentelemetry-collector.tmpl.yaml(2 hunks)pkg/controller/actions/render/template/action_render_templates.go(2 hunks)pkg/feature/manifest/types.go(2 hunks)pkg/utils/template/template.go(1 hunks)tests/e2e/monitoring_test.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- internal/controller/services/monitoring/monitoring_controller_actions.go
- api/services/v1alpha1/zz_generated.deepcopy.go
- pkg/feature/manifest/types.go
- api/services/v1alpha1/monitoring_types.go
- pkg/controller/actions/render/template/action_render_templates.go
- config/crd/bases/services.platform.opendatahub.io_monitorings.yaml
- docs/api-overview.md
- bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
- bundle/manifests/dscinitialization.opendatahub.io_dscinitializations.yaml
- config/crd/bases/dscinitialization.opendatahub.io_dscinitializations.yaml
- pkg/utils/template/template.go
- internal/controller/services/monitoring/resources/opentelemetry-collector.tmpl.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T07:56:13.158Z
Learnt from: GowthamShanmugam
PR: opendatahub-io/opendatahub-operator#2159
File: tests/e2e/creation_test.go:486-490
Timestamp: 2025-08-08T07:56:13.158Z
Learning: In tests/e2e/creation_test.go, ValidateComponentsDeploymentFailure intentionally uses an extremely restrictive ResourceQuota to simulate component deployment failures; CPU quantities must use valid forms (e.g., "1m" or "0"), since "0.1m" is invalid and causes resource.MustParse to panic.
Applied to files:
tests/e2e/monitoring_test.go
🧬 Code Graph Analysis (2)
internal/controller/services/monitoring/monitoring_controller_support.go (1)
api/services/v1alpha1/monitoring_types.go (2)
Metrics(46-51)Traces(87-99)
tests/e2e/monitoring_test.go (6)
tests/e2e/resource_options_test.go (2)
WithMutateFunc(194-198)WithMinimalObject(140-155)pkg/utils/test/testf/testf_support.go (2)
TransformPipeline(50-61)Transform(78-106)pkg/cluster/gvk/gvk.go (4)
DSCInitialization(66-70)OpenTelemetryCollector(462-466)Namespace(24-28)Monitoring(288-292)api/services/v1alpha1/monitoring_types.go (1)
Monitoring(135-141)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)internal/controller/status/status.go (1)
ConditionTypeProvisioningSucceeded(95-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: golangci-lint
- GitHub Check: Run tests and collect coverage on tests/integration
- GitHub Check: Run tests and collect coverage on internal and pkg
🔇 Additional comments (7)
bundle/manifests/services.platform.opendatahub.io_monitorings.yaml (1)
153-158: SampleRatio description and validation look consistentThe default of "0.1" and the pattern for 0..1 inclusive align with the instrumentation test expectations.
tests/e2e/monitoring_test.go (4)
15-15: Import alias for dsciv1 is appropriateBringing in dsciv1 is needed for the new helper signature.
49-49: Nice: default traces content test placed earlier in the suiteThis helps catch regressions earlier.
153-155: Retention expectation updated to 90d aligns with CRD defaultsMatches the metrics storage defaults and controller logic.
314-336: Reserved-name validation test is valuableThis ensures “otlp/tempo” cannot be overridden by users. Good negative test coverage.
internal/controller/services/monitoring/monitoring_controller_support.go (2)
32-40: Good validation: component ID regex and reserved-name checkEnforcing OpenTelemetry component ID format and rejecting reserved names early prevents invalid collector configs.
42-103: Metrics defaults handling is clear and consistent with CRDDefaults for CPU/memory requests/limits, storage size/retention, and replicas align with expectations. Using zero/empty checks on resource.Quantity.String() is reasonable.
|
/test opendatahub-operator-e2e |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2226 +/- ##
==========================================
- Coverage 40.76% 40.59% -0.18%
==========================================
Files 149 149
Lines 11949 12010 +61
==========================================
+ Hits 4871 4875 +4
- Misses 6674 6731 +57
Partials 404 404 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
den-rgb
force-pushed
the
RHOAIENG-25586
branch
from
30dcf35 to
12bae06
Compare
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/17065000618 |
den-rgb
force-pushed
the
RHOAIENG-25586
branch
from
12bae06 to
69005c1
Compare
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/17065040256 |
…iper (opendatahub-io#2317) Verifying dsc-monitoring-namespace has not been previously set Signed-off-by: Max Whittingham <[email protected]>
den-rgb
force-pushed
the
RHOAIENG-25586
branch
from
69005c1 to
df95fba
Compare
|
/test-integration |
|
ODH Operator Integration Tests #17 triggered by #2226 (comment) |
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/17065079283 |
|
/test-integration |
|
ODH Operator Integration Tests #18 triggered by #2226 (comment) |
|
/test-integration |
|
ODH Operator Integration Tests #19 triggered by #2226 (comment) |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cluster/cluster_config.go (1)
328-343: Ensure “dsc-monitoring-namespace” is never empty at runtimeOur search shows two consumers that will break if the namespace is blank:
- cmd/main.go:406 –
upgrade.CreateDefaultDSCI(ctx,…, oconfig.MonitoringNamespace)- internal/controller/dscinitialization/utils.go:137 – creating a
corev1.Namespace{Name: monitoringName}Because we bind flags (with a non‐empty default) and then use
viper.IsSet(...)to decide whether to callviper.Set(...), an explicit empty flag or env var (--dsc-monitoring-namespace="") still counts as “set” and will skip setting the fallback. As a result, both consumers will see""and fail.Please update
setManagedMonitoringNamespacein pkg/cluster/cluster_config.go (and/or add a post-Unmarshal validation) to guard on the actual string value, for example:import ( + "strings" ) func setManagedMonitoringNamespace(ctx context.Context, cli client.Client) error { - ok := viper.IsSet("dsc-monitoring-namespace") - if !ok { + ns := strings.TrimSpace(viper.GetString("dsc-monitoring-namespace")) + if ns == "" { switch platform { case ManagedRhoai, SelfManagedRhoai: viper.Set("dsc-monitoring-namespace", DefaultMonitoringNamespaceRHOAI) default: viper.Set("dsc-monitoring-namespace", DefaultMonitoringNamespaceODH) } }Optionally, validate
oc.MonitoringNamespaceimmediately afterviper.UnmarshalinLoadConfig()and report an error if it’s empty.
This ensures no consumer ever receives"".
♻️ Duplicate comments (2)
tests/e2e/monitoring_test.go (2)
278-315: Strengthen exporter assertions to validate shape (not just keys) — verify endpoints/TLS/headersAdd checks for nested fields so template indentation/helpers can’t accidentally render block scalars or mis-typed values.
Apply:
tc.EnsureResourceExists( WithMinimalObject(gvk.OpenTelemetryCollector, types.NamespacedName{Name: "data-science-collector", Namespace: dsci.Spec.Monitoring.Namespace}), WithCondition(jq.Match(`.spec.config.exporters | has("jaeger")`)), WithCondition(jq.Match(`.spec.config.exporters | has("otlp/custom")`)), WithCondition(jq.Match(`.spec.config.exporters | has("otlp/tempo")`)), // Default tempo exporter should still exist + WithCondition(jq.Match(`.spec.config.exporters.jaeger.endpoint == "%s"`, "http://jaeger-collector:14250")), + WithCondition(jq.Match(`.spec.config.exporters.jaeger.tls.insecure == %t`, true)), + WithCondition(jq.Match(`.spec.config.exporters."otlp/custom".endpoint == "%s"`, "http://custom-endpoint:4317")), + WithCondition(jq.Match(`.spec.config.exporters."otlp/custom".headers."api-key" == "%s"`, "secret-key")), WithCondition(jq.Match(`.spec.config.service.pipelines.traces.exporters | contains(["jaeger"])`)), WithCondition(jq.Match(`.spec.config.service.pipelines.traces.exporters | contains(["otlp/custom"])`)), WithCondition(jq.Match(`.spec.config.service.pipelines.traces.exporters | contains(["otlp/tempo"])`)), )
341-343: Fix compile error: unused parameter in getTempoMonolithicNameThe dsci parameter isn’t used; Go forbids unused parameters. Mark it intentionally unused.
Apply:
-func getTempoMonolithicName(dsci *dsciv1.DSCInitialization) string { +func getTempoMonolithicName(_ *dsciv1.DSCInitialization) string { return "data-science-tempomonolithic" }
🧹 Nitpick comments (5)
pkg/cluster/cluster_config.go (1)
333-340: Treat blank values as unset to avoid ending up with an empty namespaceIf the key is explicitly set to an empty string (e.g., via env), IsSet returns true and we’ll skip defaulting, leaving an unusable empty namespace. Consider also defaulting when the value is blank.
Apply:
-ok := viper.IsSet("dsc-monitoring-namespace") -if !ok { +ok := viper.IsSet("dsc-monitoring-namespace") +if !ok || strings.TrimSpace(viper.GetString("dsc-monitoring-namespace")) == "" { switch platform { case ManagedRhoai, SelfManagedRhoai: viper.Set("dsc-monitoring-namespace", DefaultMonitoringNamespaceRHOAI) default: viper.Set("dsc-monitoring-namespace", DefaultMonitoringNamespaceODH) } }bundle/manifests/dscinitialization.opendatahub.io_dscinitializations.yaml (2)
198-207: Consider making exporters map atomic to avoid partial merge surprisesMarking the map as atomic avoids strategic-merge quirks on nested arbitrary objects during updates.
Apply:
traces: description: Tracing configuration for OpenTelemetry instrumentation properties: exporters: + x-kubernetes-map-type: atomic additionalProperties: type: object x-kubernetes-preserve-unknown-fields: true description: |- Exporters defines custom trace exporters for sending traces to external observability tools. Each key represents the exporter name, and the value contains the exporter configuration. The configuration follows the OpenTelemetry Collector exporter format. type: object
198-207: Optional: enforce reserved-name validation at the CRD layer (CEL) in addition to controllerController-side validation is fine, but a CEL rule can fail-fast on invalid keys (e.g., "otlp/tempo"). If you want this defense-in-depth, we can add a validation like:
Example (subject to CEL support for map keys):
x-kubernetes-validations: - message: 'exporter name "otlp/tempo" is reserved' rule: '!("otlp/tempo" in keys(self.exporters))'If you want, I can propose the corresponding kubebuilder markers on the Go type so it’s generated into both bases/ and bundle/.
tests/e2e/monitoring_test.go (2)
49-50: Remove duplicate “Test Traces default content” from the suiteThe test case is listed twice (also at Line 55), which is redundant and lengthens the suite.
Apply:
- {"Test Traces default content", monitoringServiceCtx.ValidateMonitoringCRDefaultTracesContent},
317-339: Optional: Assert the offending reserved name appears in the error messageImproves test signal in case multiple validations are present.
Apply:
tc.EnsureResourceExists( WithMinimalObject(gvk.Monitoring, types.NamespacedName{Name: "default-monitoring"}), WithCondition(jq.Match(`.status.conditions[] | select(.type == "%s") | .status == "%s"`, status.ConditionTypeProvisioningSucceeded, metav1.ConditionFalse)), - WithCondition(jq.Match(`.status.conditions[] | select(.type == "%s") | .message | contains("reserved")`, status.ConditionTypeProvisioningSucceeded)), + WithCondition(jq.Match(`.status.conditions[] | select(.type == "%s") | .message | contains("reserved")`, status.ConditionTypeProvisioningSucceeded)), + WithCondition(jq.Match(`.status.conditions[] | select(.type == "%s") | .message | contains("otlp/tempo")`, status.ConditionTypeProvisioningSucceeded)), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
api/services/v1alpha1/monitoring_types.go(2 hunks)api/services/v1alpha1/zz_generated.deepcopy.go(3 hunks)bundle/manifests/dscinitialization.opendatahub.io_dscinitializations.yaml(1 hunks)bundle/manifests/opendatahub-operator.clusterserviceversion.yaml(1 hunks)bundle/manifests/services.platform.opendatahub.io_monitorings.yaml(1 hunks)config/crd/bases/dscinitialization.opendatahub.io_dscinitializations.yaml(1 hunks)config/crd/bases/services.platform.opendatahub.io_monitorings.yaml(1 hunks)docs/api-overview.md(31 hunks)internal/controller/services/monitoring/monitoring_controller_actions.go(1 hunks)internal/controller/services/monitoring/monitoring_controller_support.go(3 hunks)internal/controller/services/monitoring/resources/opentelemetry-collector.tmpl.yaml(2 hunks)pkg/cluster/cluster_config.go(1 hunks)pkg/controller/actions/render/template/action_render_templates.go(2 hunks)pkg/feature/manifest/types.go(2 hunks)pkg/utils/template/template.go(1 hunks)tests/e2e/monitoring_test.go(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- bundle/manifests/opendatahub-operator.clusterserviceversion.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- pkg/utils/template/template.go
- internal/controller/services/monitoring/monitoring_controller_actions.go
- pkg/feature/manifest/types.go
- pkg/controller/actions/render/template/action_render_templates.go
- api/services/v1alpha1/zz_generated.deepcopy.go
- config/crd/bases/dscinitialization.opendatahub.io_dscinitializations.yaml
- config/crd/bases/services.platform.opendatahub.io_monitorings.yaml
- api/services/v1alpha1/monitoring_types.go
- docs/api-overview.md
- internal/controller/services/monitoring/monitoring_controller_support.go
- bundle/manifests/services.platform.opendatahub.io_monitorings.yaml
- internal/controller/services/monitoring/resources/opentelemetry-collector.tmpl.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T07:56:13.158Z
Learnt from: GowthamShanmugam
PR: opendatahub-io/opendatahub-operator#2159
File: tests/e2e/creation_test.go:486-490
Timestamp: 2025-08-08T07:56:13.158Z
Learning: In tests/e2e/creation_test.go, ValidateComponentsDeploymentFailure intentionally uses an extremely restrictive ResourceQuota to simulate component deployment failures; CPU quantities must use valid forms (e.g., "1m" or "0"), since "0.1m" is invalid and causes resource.MustParse to panic.
Applied to files:
tests/e2e/monitoring_test.go
🧬 Code Graph Analysis (2)
pkg/cluster/cluster_config.go (1)
pkg/cluster/const.go (4)
ManagedRhoai(7-7)SelfManagedRhoai(9-9)DefaultMonitoringNamespaceRHOAI(21-21)DefaultMonitoringNamespaceODH(19-19)
tests/e2e/monitoring_test.go (5)
tests/e2e/resource_options_test.go (3)
WithMutateFunc(194-198)WithMinimalObject(140-155)WithCondition(203-207)pkg/utils/test/testf/testf_support.go (2)
TransformPipeline(50-61)Transform(78-106)pkg/cluster/gvk/gvk.go (3)
DSCInitialization(66-70)OpenTelemetryCollector(462-466)Namespace(24-28)pkg/utils/test/matchers/jq/jq_matcher.go (1)
Match(11-15)internal/controller/status/status.go (1)
ConditionTypeProvisioningSucceeded(95-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build/push catalog image
- GitHub Check: Run tests and collect coverage on internal and pkg
- GitHub Check: Run tests and collect coverage on tests/integration
- GitHub Check: golangci-lint
🔇 Additional comments (5)
pkg/cluster/cluster_config.go (1)
333-340: Good guard: avoid clobbering user-provided monitoring namespaceUsing viper.IsSet before defaulting preserves explicit configuration. This aligns with the PR’s intent.
bundle/manifests/dscinitialization.opendatahub.io_dscinitializations.yaml (1)
198-207: Schema addition for traces.exporters looks correct and forward-compatibleadditionalProperties with x-kubernetes-preserve-unknown-fields allows arbitrary exporter configs, matching OTel Collector expectations.
tests/e2e/monitoring_test.go (3)
156-157: Retention expectation updated to 90d — LGTMMatches the CRD default reflected in the bundle.
374-375: Default exporters map in setMonitoringTraces — LGTMCreating an empty map is harmless and lets callers add exporters explicitly; template code will simply render none.
429-430: Passing dsci through to getTempoMonolithicName — LGTMSignature alignment is correct; once the parameter is marked unused, this remains valid.
| // Set metrics to empty object | ||
| tc.EventuallyResourceCreatedOrUpdated( | ||
| WithMinimalObject(gvk.DSCInitialization, tc.DSCInitializationNamespacedName), | ||
| WithMutateFunc(testf.Transform(`.spec.monitoring = %s`, `{metrics: {}, managementState: "Managed"}`)), | ||
| WithMutateFunc(testf.TransformPipeline( | ||
| testf.Transform(`.spec.monitoring.metrics = %s`, `{metrics: {}, managementState: "Managed"}`), | ||
| testf.Transform(`.spec.monitoring.traces = null`), | ||
| )), |
There was a problem hiding this comment.
Broken mutate payload: sets .spec.monitoring.metrics to an object containing “metrics” and “managementState”
This produces an invalid shape under metrics (a nested “metrics” field) and will likely be rejected or misinterpreted. Intent appears to be clearing metrics and traces while keeping monitoring Managed.
Apply:
- WithMutateFunc(testf.TransformPipeline(
- testf.Transform(`.spec.monitoring.metrics = %s`, `{metrics: {}, managementState: "Managed"}`),
- testf.Transform(`.spec.monitoring.traces = null`),
- )),
+ WithMutateFunc(testf.TransformPipeline(
+ testf.Transform(`.spec.monitoring.managementState = "%s"`, "Managed"),
+ testf.Transform(`.spec.monitoring.metrics = null`),
+ testf.Transform(`.spec.monitoring.traces = null`),
+ )),📝 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.
| // Set metrics to empty object | |
| tc.EventuallyResourceCreatedOrUpdated( | |
| WithMinimalObject(gvk.DSCInitialization, tc.DSCInitializationNamespacedName), | |
| WithMutateFunc(testf.Transform(`.spec.monitoring = %s`, `{metrics: {}, managementState: "Managed"}`)), | |
| WithMutateFunc(testf.TransformPipeline( | |
| testf.Transform(`.spec.monitoring.metrics = %s`, `{metrics: {}, managementState: "Managed"}`), | |
| testf.Transform(`.spec.monitoring.traces = null`), | |
| )), | |
| // Set metrics to empty object | |
| tc.EventuallyResourceCreatedOrUpdated( | |
| WithMinimalObject(gvk.DSCInitialization, tc.DSCInitializationNamespacedName), | |
| WithMutateFunc(testf.TransformPipeline( | |
| testf.Transform(`.spec.monitoring.managementState = "%s"`, "Managed"), | |
| testf.Transform(`.spec.monitoring.metrics = null`), | |
| testf.Transform(`.spec.monitoring.traces = null`), | |
| )), |
🤖 Prompt for AI Agents
In tests/e2e/monitoring_test.go around lines 209 to 215, the mutate payload
incorrectly sets .spec.monitoring.metrics to an object containing "metrics" and
"managementState" (creating a nested metrics field); change the transforms so
that .spec.monitoring.metrics is set to an empty object and managementState
remains "Managed" at the correct path (e.g. set .spec.monitoring.metrics = {}
and .spec.monitoring.managementState = "Managed"), and keep
.spec.monitoring.traces = null; update the testf.Transform calls accordingly so
the resulting JSON shape is valid.
This PR can't be merged just yet 😢Please run For more info: https://github.com/opendatahub-io/opendatahub-operator/actions/runs/17065577372 |
|
/test-integration |
|
ODH Operator Integration Tests #20 triggered by #2226 (comment) |
Description
https://issues.redhat.com/browse/RHOAIENG-25586
How Has This Been Tested?
Screenshot or short clip
Merge criteria
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests