feat(evalhub): expose OTEL configuration in CR spec (RHOAIENG-51312)#653
Merged
ruivieira merged 1 commit intotrustyai-explainability:mainfrom Feb 27, 2026
Merged
Conversation
- Allow users to configure OpenTelemetry via spec.otel instead of manually editing the generated ConfigMap. Presence of the field implies OTEL is enabled; omitting it keeps OTEL disabled. - Follows the same pattern as spec.database for conditional config generation. SamplingRatio uses a string type due to controller-gen CRD constraints and is parsed to float64 at reconcile time.
Contributor
📝 WalkthroughWalkthroughThis change introduces OpenTelemetry (OTEL) configuration support to EvalHub. The update adds an OTELSpec type to the API, extends the Kubernetes CRD schema with optional OTEL configuration properties, and implements controller logic to parse and populate OTEL settings into the generated configuration file. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ppadashe-psp 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
api/evalhub/v1alpha1/evalhub_types.go (1)
46-50: Add CRD-level validation marker forsamplingRatio.The field documentation says the value must be between
0and1, but schema markers currently do not enforce that constraint.✅ Suggested marker
// Trace sampling ratio as a string-encoded float between "0" and "1" (e.g. "0.5"). // Defaults to "1.0" (sample everything) when omitted. // +kubebuilder:default:="1.0" + // +kubebuilder:validation:Pattern=`^(0(\.\d+)?|1(\.0+)?)$` // +optional SamplingRatio string `json:"samplingRatio,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/evalhub/v1alpha1/evalhub_types.go` around lines 46 - 50, Add a kubebuilder validation marker on the SamplingRatio field to enforce it is a string-encoded float between 0 and 1; specifically add a +kubebuilder:validation:Pattern marker that matches values like "0", "0.x", and "1" (e.g. pattern ^(0(\.\d+)?|1(\.0+)?)$) alongside the existing +kubebuilder:default and +optional markers so the CRD schema validates the constraint for the SamplingRatio field.controllers/evalhub/unit_test.go (1)
1167-1266: Add negative-path OTEL sampling tests.This block currently validates only happy paths. Please add cases for invalid
samplingRatioformat and out-of-range values to prevent regressions in validation logic.🧪 Suggested test additions
func TestGenerateConfigData_WithOTEL(t *testing.T) { @@ t.Run("should omit otel section when not configured", func(t *testing.T) { @@ assert.Nil(t, config.OTEL) }) + + t.Run("should fail when samplingRatio is not numeric", func(t *testing.T) { + evalHub := &evalhubv1alpha1.EvalHub{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-evalhub", + Namespace: "test-namespace", + }, + Spec: evalhubv1alpha1.EvalHubSpec{ + Otel: &evalhubv1alpha1.OTELSpec{ + SamplingRatio: "not-a-number", + }, + }, + } + + reconciler := &EvalHubReconciler{Scheme: scheme} + _, err := reconciler.generateConfigData(evalHub) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid samplingRatio") + }) + + t.Run("should fail when samplingRatio is out of range", func(t *testing.T) { + evalHub := &evalhubv1alpha1.EvalHub{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-evalhub", + Namespace: "test-namespace", + }, + Spec: evalhubv1alpha1.EvalHubSpec{ + Otel: &evalhubv1alpha1.OTELSpec{ + SamplingRatio: "1.5", + }, + }, + } + + reconciler := &EvalHubReconciler{Scheme: scheme} + _, err := reconciler.generateConfigData(evalHub) + require.Error(t, err) + assert.Contains(t, err.Error(), "samplingRatio") + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/evalhub/unit_test.go` around lines 1167 - 1266, Add negative-path subtests inside TestGenerateConfigData_WithOTEL to cover invalid OTEL sampling values: create t.Run("invalid sampling format") where EvalHub.Spec.Otel.SamplingRatio = "not-a-number" and assert reconciler.generateConfigData(evalHub) returns a non-nil error; and t.Run("sampling out of range") where SamplingRatio is set to values like "-0.1" and "1.5" (two subcases) and assert generateConfigData returns an error for each. Reference the EvalHubReconciler.generateConfigData call, evalhubv1alpha1.OTELSpec.SamplingRatio field, and the EvalHubConfig/OTEL types when locating the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controllers/evalhub/configmap.go`:
- Around line 165-170: The parsing of instance.Spec.Otel.SamplingRatio in the
function that builds otelCfg currently accepts invalid values; after
strconv.ParseFloat on instance.Spec.Otel.SamplingRatio (the parsed variable
named ratio) add validation to ensure the value is finite and inside [0,1] by
checking math.IsNaN(ratio) and math.IsInf(ratio, 0) and that ratio >= 0 && ratio
<= 1, and return a clear fmt.Errorf if validation fails before assigning
otelCfg.SamplingRatio = &ratio.
---
Nitpick comments:
In `@api/evalhub/v1alpha1/evalhub_types.go`:
- Around line 46-50: Add a kubebuilder validation marker on the SamplingRatio
field to enforce it is a string-encoded float between 0 and 1; specifically add
a +kubebuilder:validation:Pattern marker that matches values like "0", "0.x",
and "1" (e.g. pattern ^(0(\.\d+)?|1(\.0+)?)$) alongside the existing
+kubebuilder:default and +optional markers so the CRD schema validates the
constraint for the SamplingRatio field.
In `@controllers/evalhub/unit_test.go`:
- Around line 1167-1266: Add negative-path subtests inside
TestGenerateConfigData_WithOTEL to cover invalid OTEL sampling values: create
t.Run("invalid sampling format") where EvalHub.Spec.Otel.SamplingRatio =
"not-a-number" and assert reconciler.generateConfigData(evalHub) returns a
non-nil error; and t.Run("sampling out of range") where SamplingRatio is set to
values like "-0.1" and "1.5" (two subcases) and assert generateConfigData
returns an error for each. Reference the EvalHubReconciler.generateConfigData
call, evalhubv1alpha1.OTELSpec.SamplingRatio field, and the EvalHubConfig/OTEL
types when locating the code to modify.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/evalhub/v1alpha1/evalhub_types.goapi/evalhub/v1alpha1/zz_generated.deepcopy.goconfig/crd/bases/trustyai.opendatahub.io_evalhubs.yamlcontrollers/evalhub/configmap.gocontrollers/evalhub/configmap_test.gocontrollers/evalhub/unit_test.go
Comment on lines
+165
to
+170
| if instance.Spec.Otel.SamplingRatio != "" { | ||
| ratio, err := strconv.ParseFloat(instance.Spec.Otel.SamplingRatio, 64) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid samplingRatio %q: %w", instance.Spec.Otel.SamplingRatio, err) | ||
| } | ||
| otelCfg.SamplingRatio = &ratio |
Contributor
There was a problem hiding this comment.
Validate samplingRatio bounds and finite values before emitting config.
Current logic accepts values like -1, 1.5, NaN, or Inf. OTEL sampling should be constrained to finite values in [0,1].
🛠️ Suggested fix
import (
"context"
"fmt"
+ "math"
"strconv"
"strings"
@@
- if instance.Spec.Otel.SamplingRatio != "" {
- ratio, err := strconv.ParseFloat(instance.Spec.Otel.SamplingRatio, 64)
+ samplingRatio := strings.TrimSpace(instance.Spec.Otel.SamplingRatio)
+ if samplingRatio != "" {
+ ratio, err := strconv.ParseFloat(samplingRatio, 64)
if err != nil {
return nil, fmt.Errorf("invalid samplingRatio %q: %w", instance.Spec.Otel.SamplingRatio, err)
}
+ if math.IsNaN(ratio) || math.IsInf(ratio, 0) || ratio < 0 || ratio > 1 {
+ return nil, fmt.Errorf("invalid samplingRatio %q: must be a finite number between 0 and 1", instance.Spec.Otel.SamplingRatio)
+ }
otelCfg.SamplingRatio = &ratio
}📝 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
| if instance.Spec.Otel.SamplingRatio != "" { | |
| ratio, err := strconv.ParseFloat(instance.Spec.Otel.SamplingRatio, 64) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid samplingRatio %q: %w", instance.Spec.Otel.SamplingRatio, err) | |
| } | |
| otelCfg.SamplingRatio = &ratio | |
| samplingRatio := strings.TrimSpace(instance.Spec.Otel.SamplingRatio) | |
| if samplingRatio != "" { | |
| ratio, err := strconv.ParseFloat(samplingRatio, 64) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid samplingRatio %q: %w", instance.Spec.Otel.SamplingRatio, err) | |
| } | |
| if math.IsNaN(ratio) || math.IsInf(ratio, 0) || ratio < 0 || ratio > 1 { | |
| return nil, fmt.Errorf("invalid samplingRatio %q: must be a finite number between 0 and 1", instance.Spec.Otel.SamplingRatio) | |
| } | |
| otelCfg.SamplingRatio = &ratio | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controllers/evalhub/configmap.go` around lines 165 - 170, The parsing of
instance.Spec.Otel.SamplingRatio in the function that builds otelCfg currently
accepts invalid values; after strconv.ParseFloat on
instance.Spec.Otel.SamplingRatio (the parsed variable named ratio) add
validation to ensure the value is finite and inside [0,1] by checking
math.IsNaN(ratio) and math.IsInf(ratio, 0) and that ratio >= 0 && ratio <= 1,
and return a clear fmt.Errorf if validation fails before assigning
otelCfg.SamplingRatio = &ratio.
ruivieira
merged commit 3b703cf
into
trustyai-explainability:main
9 of 10 checks passed
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.
Summary by CodeRabbit
New Features
Tests