-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[OPIK-8040] [BE] fix: apply online evaluation sampling to production traces only #7957
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6e1b3f7
c8c9dec
69cd1b5
30a06e4
7513edd
ad28431
be6c9f4
9fd8f74
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,8 @@ | |
| import io.swagger.v3.oas.annotations.media.DiscriminatorMapping; | ||
| import io.swagger.v3.oas.annotations.media.Schema; | ||
| import jakarta.validation.Valid; | ||
| import jakarta.validation.constraints.DecimalMax; | ||
| import jakarta.validation.constraints.DecimalMin; | ||
| import jakarta.validation.constraints.NotBlank; | ||
| import jakarta.validation.constraints.NotNull; | ||
| import jakarta.validation.constraints.Size; | ||
|
|
@@ -54,7 +56,8 @@ public abstract sealed class AutomationRuleEvaluatorUpdate<T, E extends Filter> | |
| // the API boundary instead of failing the update (OPIK-7371). | ||
| @NotBlank @Size(max = 150, message = "cannot exceed 150 characters") private final String name; | ||
|
|
||
| private final float samplingRate; | ||
| @Schema(description = "Fraction of production (SDK-logged) items this rule scores, from 0 to 1. Trace rules ignore this value for experiment, playground and optimization traces and score them in full; span and thread rules only ever evaluate SDK-logged data.") | ||
| @DecimalMin("0") @DecimalMax("1") private final float samplingRate; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] New validation guard has no test on the update path The new @DecimalMin("0")/@DecimalMax("1") guard is a semantically distinct validation branch on a public endpoint and nothing exercises it: no test asserts 422 for sampling_rate < 0 or > 1, nor 2xx for the inclusive boundaries 0 and 1. The sibling constraint on this very class (@SiZe(max = 150) on name, line 57) got dedicated 422 assertions for both create and update (AutomationRuleEvaluatorsResourceTest.java:959 and :979), so this change lacks the parity the team already established. 💡 Add a @ParameterizedTest over {-0.1, 1.1} asserting SC_UNPROCESSABLE_ENTITY (with the violation message) and over {0f, 1f} asserting success, for both POST /v1/private/automations/evaluators and PATCH/PUT of an existing rule. rule:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commit 9fd8f74 addressed this comment by adding parameterized create and update tests for out-of-range sampling rates and inclusive 0/1 boundaries, including validation error and success assertions.
jverre marked this conversation as resolved.
|
||
|
|
||
| @Builder.Default | ||
| private final boolean enabled = true; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.