fix(alerts): accept window_in_seconds as the threshold window - #7927
fix(alerts): accept window_in_seconds as the threshold window#7927BetterAndBetterII wants to merge 2 commits into
Conversation
Add a MetricsAlertJob regression for TRACE_ERRORS configs that supply the documented REST key window_in_seconds instead of the evaluator's window key, so a silent 201 can no longer hide a missing-window job failure. Co-authored-by: Yuzhong Zhang <BetterAndBetterII@users.noreply.github.com>
MetricsAlertJob read only the canonical window key. Map the documented REST alias onto that value so stored threshold:errors configs evaluate instead of failing every scheduled run. Co-authored-by: Yuzhong Zhang <BetterAndBetterII@users.noreply.github.com>
github-actions
Bot
added
java
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| BigDecimal threshold = new BigDecimal(thresholdString); | ||
|
|
||
| var windowString = config.configValue().get(WINDOW_CONFIG_KEY); | ||
| var windowString = resolveWindow(config.configValue()); |
There was a problem hiding this comment.
Misleading missing-config diagnostics
resolveWindow(config.configValue()) reports only window when both aliases are absent, so the job failure omits the accepted window_in_seconds spelling and misleads users — should we mention both keys or report the requirement generically?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/jobs/MetricsAlertJob.java`
around lines 429-434, update the missing-window validation in `buildTriggerConfig`.
Because `resolveWindow(config.configValue())` accepts both `window` and
`window_in_seconds`, change the `IllegalArgumentException` message to mention both
accepted keys (or describe the resolved window configuration generically) so failures
are accurate.
| // Documented REST alias for WINDOW_CONFIG_KEY; stored configs may use either spelling. | ||
| public static final String WINDOW_IN_SECONDS_CONFIG_KEY = "window_in_seconds"; |
There was a problem hiding this comment.
alertTriggersToFormTriggers reads only config_value.window, so alias-only window_in_seconds alerts produce forms without required window; formTriggersToAlertTriggers likewise serializes only window, which can emit trigger_configs: [] and cause AlertService.update to wipe existing configuration — should we resolve window_in_seconds with window taking precedence in both projections? configValue is documented only as Map<String,String>, so REST consumers cannot discover the window_in_seconds alias or its precedence over window_seconds — should we add that to the source-level schema/API docs rather than generated OpenAPI artifacts?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/main/java/com/comet/opik/api/AlertTriggerConfig.java` around
lines 46-59, where `window_in_seconds` is introduced as an alias: 1) Update
`alertTriggersToFormTriggers` and `formTriggersToAlertTriggers` to consistently resolve
`window` first and fall back to `window_in_seconds` (matching `resolveWindow`'s
precedence), applying this to both simple threshold and grouped feedback-score
conditions, and serialize the effective value so alias-backed alerts remain editable and
never collapse into an empty `trigger_configs` list. 2) Document the `window_in_seconds`
alias in the source-level API/schema documentation for `configValue`, explaining that
both `window` and `window_in_seconds` are accepted and that `window` takes precedence
when both are provided; do not manually edit generated OpenAPI artifacts—regenerate
them after merge.
| public static String resolveWindow(Map<String, String> configValue) { | ||
| if (configValue == null) { | ||
| return null; |
There was a problem hiding this comment.
Undeclared nullable API contract
resolveWindow accepts a null configValue and returns null, but its annotations expose neither nullable contract to callers or static analysis — should we add the project’s nullable annotations, such as @Nullable Map<String, String> and @Nullable String, while preserving the raw-null behavior?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/main/java/com/comet/opik/api/AlertTriggerConfig.java` around
lines 53-55, update the `resolveWindow` method to document its explicit nullable
contract. Annotate the `configValue` parameter and the `String` return value with the
project’s standard nullable annotation, adding the necessary import, while preserving
the current raw-null behavior.
| String window = configValue.get(WINDOW_CONFIG_KEY); | ||
| return window != null ? window : configValue.get(WINDOW_IN_SECONDS_CONFIG_KEY); |
There was a problem hiding this comment.
Blank canonical window breaks alert evaluation
resolveWindow preserves window: "" because it falls back only when window == null, so MetricsAlertJob.buildTriggerConfig passes a blank value to Long.parseLong(windowString) and alert evaluation fails instead of using window_in_seconds: "300". Should we treat blank/whitespace canonical values as absent and fall back to the alias, or have AlertService reject this invalid combination?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/main/java/com/comet/opik/api/AlertTriggerConfig.java` around
lines 57-58, update `resolveWindow` so blank or whitespace-only `window` values do not
take precedence over a valid `window_in_seconds` alias. Trim and validate the canonical
value before returning it, then fall back to the alias when the canonical value is
blank; alternatively, explicitly reject the combination if that is the established
config contract, and ensure the behavior prevents `Long.parseLong` from receiving an
empty value.
| @ParameterizedTest | ||
| @MethodSource("windowConfigKeys") | ||
| void firesTraceErrorsWhenWindowProvidedUnderCanonicalOrDocumentedAlias(String windowKey) { | ||
| Alert alert = alertWithErrorThreshold(windowKey, "2", "300"); | ||
|
|
||
| when(projectMetricsDAO.getTotalTraceErrors(anyList(), any(Instant.class), any(Instant.class))) | ||
| .thenReturn(Mono.just(new BigDecimal("3"))); | ||
| when(alertService.findAllByWorkspaceAndEventTypes(null, | ||
| MetricsAlertJob.SUPPORTED_EVENT_TYPES)).thenReturn(List.of(alert)); | ||
|
|
||
| job.doJob(null); | ||
|
|
There was a problem hiding this comment.
Four metric aliases remain unverified
windowConfigKeys() covers only TRACE_ERRORS, so regressions in the window_in_seconds alias path for the four other supported event types can go undetected while tests pass — should we extend the behavior-focused matrix or integration coverage to assert each expected interval calculation, per .agents/skills/opik-backend/testing.md?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/jobs/MetricsAlertJobTest.java
around lines 234-245, expand
`firesTraceErrorsWhenWindowProvidedUnderCanonicalOrDocumentedAlias` beyond the
`TRACE_ERRORS` fixture to cover `TRACE_COST`, `TRACE_LATENCY`, `TRACE_FEEDBACK_SCORE`,
and `TRACE_THREAD_FEEDBACK_SCORE`. Update the parameter matrix and test fixture/mocks
with the required event-specific threshold configuration and metric branches, then
assert that each case uses the alias to calculate the expected interval and emits the
correct payload.
| private static Alert alertWithErrorThreshold(String windowKey, String threshold, String window) { | ||
| AlertTrigger trigger = AlertTrigger.builder() | ||
| .id(UUID.randomUUID()) | ||
| .eventType(AlertEventType.TRACE_ERRORS) | ||
| .triggerConfigs(List.of(AlertTriggerConfig.builder() | ||
| .id(UUID.randomUUID()) | ||
| .type(AlertTriggerConfigType.THRESHOLD_ERRORS) | ||
| .configValue(Map.of( | ||
| THRESHOLD_CONFIG_KEY, threshold, | ||
| windowKey, window)) |
There was a problem hiding this comment.
Window alias precedence is untested
The fixture sets exactly one window key, so its cases never exercise resolveWindow's precedence and a regression favoring window_in_seconds would still pass — should we add a focused case with both values different and assert the webhook uses window?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/jobs/MetricsAlertJobTest.java`
around lines 306-315, update the `alertWithErrorThreshold` fixture and related
parameterized tests to cover configurations containing both `window` and
`window_in_seconds` with different values. Add a focused assertion that the webhook
payload uses the canonical `window` value, ensuring regressions that prefer the
documented alias are caught.
2 participants
Summary
threshold:errorsconfigs stored aswindow_in_seconds(the documented REST spelling) were accepted with 201, then everyMetricsAlertJobrun failed looking forwindow.AlertTriggerConfig.resolveWindow()preferswindow, thenwindow_in_seconds. Canonicalwindowis unchanged.window_in_secondsforthreshold:errors, evaluator requireswindow; alert still never fires after the fix #7862. Does not change Part 2 (alert still not firing after the key is corrected).Test plan
window_in_secondsfires the sameTRACE_ERRORSwebhook aswindowwindowstill worksmvn test -Dtest=MetricsAlertJobTest— 13 passed