Skip to content

fix(alerts): accept window_in_seconds as the threshold window #7927

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.

Sign up for GitHub

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

Open
BetterAndBetterII wants to merge 2 commits into
base: main
Choose a base branch
from
+65 −1
Open

fix(alerts): accept window_in_seconds as the threshold window #7927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
215044f
[NA] [BE] test: cover window_in_seconds alias on threshold:errors alerts
cursoragent Aug 20, 2026
dfc1897
fix(be): accept window_in_seconds as threshold alert window alias
cursoragent Aug 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions apps/opik-backend/src/main/java/com/comet/opik/api/AlertTriggerConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,18 @@ public record AlertTriggerConfig(
public static final String PROJECT_IDS_CONFIG_KEY = "project_ids";
public static final String THRESHOLD_CONFIG_KEY = "threshold";
public static final String WINDOW_CONFIG_KEY = "window";
// 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";
Comment on lines +46 to +47

@baz-reviewer baz-reviewer Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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 final String NAME_CONFIG_KEY = "name";
public static final String OPERATOR_CONFIG_KEY = "operator";
// Comma-separated GuardrailType names (e.g. "PII,TOPIC"); empty/absent means all types.
public static final String GUARDRAIL_TYPES_CONFIG_KEY = "guardrail_types";

public static String resolveWindow(Map<String, String> configValue) {
if (configValue == null) {
return null;
Comment on lines +53 to +55

@baz-reviewer baz-reviewer Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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);
Comment on lines +57 to +58

@baz-reviewer baz-reviewer Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.

}
}
3 changes: 2 additions & 1 deletion apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/jobs/MetricsAlertJob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import static com.comet.opik.api.AlertTriggerConfig.OPERATOR_CONFIG_KEY;
import static com.comet.opik.api.AlertTriggerConfig.THRESHOLD_CONFIG_KEY;
import static com.comet.opik.api.AlertTriggerConfig.WINDOW_CONFIG_KEY;
import static com.comet.opik.api.AlertTriggerConfig.resolveWindow;

/**
* Scheduled job for processing metrics-based alerts.
Expand Down Expand Up @@ -425,7 +426,7 @@ private TriggerConfig buildTriggerConfig(com.comet.opik.api.AlertTriggerConfig c
}
BigDecimal threshold = new BigDecimal(thresholdString);

var windowString = config.configValue().get(WINDOW_CONFIG_KEY);
var windowString = resolveWindow(config.configValue());

@baz-reviewer baz-reviewer Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.

if (windowString == null) {
throw new IllegalArgumentException(
"Missing config value for key '%s' in trigger of type '%s'"
Expand Down
53 changes: 53 additions & 0 deletions .../opik-backend/src/test/java/com/comet/opik/api/resources/v1/jobs/MetricsAlertJobTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;
import org.junit.jupiter.params.provider.MethodSource;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
Expand All @@ -32,6 +33,7 @@
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.stream.Stream;

import static com.comet.opik.api.AlertTriggerConfig.NAME_CONFIG_KEY;
import static com.comet.opik.api.AlertTriggerConfig.OPERATOR_CONFIG_KEY;
Expand Down Expand Up @@ -229,6 +231,33 @@ void payloadScalarsMatchSourceOrderEvenWhenSecondFetchCompletesFirst(AlertEventT
assertThat(payload.get("conditions").get(1).get("threshold").asText()).isEqualTo("0.6000");
}

@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);

Comment on lines +234 to +245

@baz-reviewer baz-reviewer Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

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

Fix in Cursor

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.

ArgumentCaptor<List<String>> payloadCaptor = listCaptor();
verify(alertWebhookSender, timeout(ASYNC_TIMEOUT_MS)).createAndSendWebhook(
any(), eq(WORKSPACE_ID), anyString(), eq(AlertEventType.TRACE_ERRORS), anyList(),
payloadCaptor.capture(), anyList());

JsonNode payload = JsonUtils.readValue(payloadCaptor.getValue().getFirst(), JsonNode.class);
assertThat(payload.get("window_seconds").asLong()).isEqualTo(300L);
assertThat(payload.get("metric_value").asText()).isEqualTo("3");
assertThat(payload.get("threshold").asText()).isEqualTo("2");
}

static Stream<String> windowConfigKeys() {
return Stream.of(WINDOW_CONFIG_KEY, "window_in_seconds");
}

@Test
void doesNotEvaluateWhenInterrupted() throws org.quartz.UnableToInterruptJobException {
job.interrupt();
Expand Down Expand Up @@ -274,6 +303,30 @@ private static Alert alertWithGroupedFeedbackConfigs(AlertEventType eventType, i
.build();
}

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))
Comment on lines +306 to +315

@baz-reviewer baz-reviewer Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

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.

.build()))
.build();

return Alert.builder()
.id(UUID.randomUUID())
.name("test-alert")
.enabled(true)
.webhook(Webhook.builder().url("http://example/hook").build())
.triggers(List.of(trigger))
.projectId(PROJECT_ID)
.workspaceId(WORKSPACE_ID)
.build();
}

private static AlertTriggerConfig feedbackConfig(String operator, String threshold, Integer groupIndex) {
return AlertTriggerConfig.builder()
.id(UUID.randomUUID())
Expand Down