[NA] [BE][FE] feat: Add Feishu as native alert destination#5775
[NA] [BE][FE] feat: Add Feishu as native alert destination#5775rusherman wants to merge 2 commits intocomet-ml:mainfrom
Conversation
Add Feishu/Lark as the fourth native alert type alongside General, Slack, and PagerDuty. Uses Feishu Interactive Card format for rich alert display in group chats via custom bot webhooks. Backend: - DB migration to add 'feishu' to alert_type ENUM - FEISHU enum value in AlertType - Feishu card payload model classes (6 files) - FeishuWebhookPayloadMapper handling all 10 event types - AlertPayloadAdapter integration - Unit tests (36 tests, all passing) Frontend: - ALERT_TYPE enum extended with 'feishu' - Feishu SVG icon - Labels, icons, and field mappings in helpers - DestinationSelector updated
| private static String buildContent(@NonNull WebhookEvent<Map<String, Object>> event) { | ||
| List<?> metadata = (List<?>) event.getPayload().getOrDefault("metadata", List.of()); | ||
| int count = metadata.size(); | ||
| String eventTypeName = formatEventType(event.getEventType()); | ||
|
|
||
| String summary = String.format("**%d** new %s event%s happened", | ||
| count, eventTypeName, count != 1 ? "s" : ""); | ||
|
|
||
| var metadataUrl = event.getAlertMetadata().getOrDefault(BASE_URL_METADATA_KEY, DEFAULT_BASE_URL); | ||
| metadataUrl = metadataUrl.endsWith("/") ? metadataUrl : metadataUrl + "/"; | ||
| String baseUrl = metadataUrl + event.getWorkspaceName(); |
There was a problem hiding this comment.
metadata and metadataUrl can be null if the payload contains explicit null values, causing NPE on metadata.size() and metadataUrl.endsWith("/") — should we normalize both with Optional.ofNullable(...).orElse(...) at extraction (lines 75-84 and 108-115 in buildContent and buildActionUrl)?
Finding types: Explicit null contracts | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/webhooks/feishu/FeishuWebhookPayloadMapper.java
around lines 75 to 84 (buildContent method) and lines 108 to 115 (buildActionUrl
method), the code uses event.getPayload().getOrDefault("metadata", List.of()) and
event.getAlertMetadata().getOrDefault(BASE_URL_METADATA_KEY, DEFAULT_BASE_URL) which can
return null if the payload explicitly contains "metadata": null or base_url->null,
leading to NPE when calling metadata.size() or metadataUrl.endsWith("/"). Replace the
metadata assignment with var metadata = Optional.ofNullable((List<?>)
event.getPayload().get("metadata")).orElse(List.of()) in both methods. Replace the
metadataUrl assignment with var metadataUrl =
Optional.ofNullable(event.getAlertMetadata().get(BASE_URL_METADATA_KEY)).orElse(DEFAULT_BASE_URL)
(or ObjectUtils.defaultIfNull(...)) in both methods before calling endsWith. Apply these
changes consistently in both buildContent and buildActionUrl to ensure downstream logic
always receives non-null values.
| private static String formatMetricsAlertPayload(@NonNull MetricsAlertPayload payload, String type) { | ||
| try { | ||
| String windowDuration = formatWindowDuration(payload.windowSeconds()); | ||
|
|
||
| String scope = (payload.projectIds() == null || payload.projectIds().isEmpty()) | ||
| ? "*Workspace-wide*" | ||
| : TemplateUtils.newST(PROJECTS_TEMPLATE) | ||
| .add("projectNames", payload.projectNames()) | ||
| .render(); | ||
|
|
||
| String valuePrefix = type.equals("Cost") ? "$" : ""; | ||
| String valueSuffix = type.equals("Latency") ? " s" : ""; |
There was a problem hiding this comment.
formatMetricsAlertPayload catches Exception and hides unrelated failures, should we catch the template-specific exceptions from TemplateUtils.newST(...).render() (e.g. IllegalArgumentException) or at least add a TODO if the broad catch must remain?
Finding type: Use specific exceptions | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/webhooks/feishu/FeishuWebhookPayloadMapper.java
around lines 231 to 261, the method formatMetricsAlertPayload currently catches
java.lang.Exception which masks unrelated failures. Replace the broad catch with
specific exception types thrown by the template/rendering path (for example
IllegalArgumentException and the template library's rendering exception — or
TemplateException if applicable), logging the error as now and returning the fallback
string only for those specific cases. If you must temporarily keep a broad catch while
we confirm the exact library exceptions, add a TODO above the catch that lists the
follow-up: replace the broad catch with the exact template/render exceptions (e.g.,
TemplateException, IllegalArgumentException) and include a short justification and
ticket reference.
| private static String formatWindowDuration(long seconds) { | ||
| Duration duration = Duration.ofSeconds(seconds); | ||
|
|
||
| if (duration.toDays() > 0) { | ||
| long days = duration.toDays(); |
There was a problem hiding this comment.
formatWindowDuration is duplicated across mappers — should we extract AlertFormattingUtils.formatWindowDuration and reuse it?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
| private static String formatEventType(@NonNull AlertEventType eventType) { | ||
| return switch (eventType) { | ||
| case PROMPT_CREATED -> "Prompt Created"; | ||
| case PROMPT_DELETED -> "Prompt Deleted"; | ||
| case PROMPT_COMMITTED -> "Prompt Committed"; |
There was a problem hiding this comment.
formatEventType duplicates SlackWebhookPayloadMapper.formatEventType, should we extract the switch into AlertEventDescriptions.formatEventType and reuse it from both mappers?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
| ALTER TABLE alerts | ||
| MODIFY COLUMN alert_type ENUM('general', 'slack', 'pagerduty', 'feishu') NOT NULL DEFAULT 'general'; | ||
|
|
||
| --rollback ALTER TABLE alerts MODIFY COLUMN alert_type ENUM('general', 'slack', 'pagerduty') NOT NULL DEFAULT 'general'; |
There was a problem hiding this comment.
The rollback tries to remove feishu from the enum but will fail if rows exist with alert_type = 'feishu', should we mark this migration forward-only and document remediation like -- forward-only; restore from backup or delete feishu rows before reverting?
Finding type: Migration rollback safety | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In
apps/opik-backend/src/main/resources/liquibase/db-app-state/migrations/000059_add_feishu_alert_type.sql
around lines 5-8, the rollback currently tries to remove the 'feishu' enum value which
will fail if any rows use it. Replace the rollback line with a documented forward-only
note: remove the rollback SQL and add a comment like '-- forward-only: cannot safely
remove enum value; to revert, restore from backup or first migrate/delete rows with
alert_type = ''feishu'' before altering the column'. Ensure the changeset is clearly
marked as irreversible and include suggested remediation steps so operators know how to
safely revert if needed.
| public enum AlertType { | ||
| GENERAL("general"), | ||
| SLACK("slack"), | ||
| PAGERDUTY("pagerduty"); | ||
| PAGERDUTY("pagerduty"), | ||
| FEISHU("feishu"); |
There was a problem hiding this comment.
AlertType added FEISHU but the OpenAPI/SDKs weren't regenerated so clients will throw on alert_type: "feishu"; should we update the API/SDKs or document compatibility and allow unknown enum members?
Finding type: Breaking Changes | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In apps/opik-backend/src/main/java/com/comet/opik/api/AlertType.java around lines 12-16,
the enum AlertType was extended with FEISHU which will break existing SDKs and OpenAPI
consumers. Update the API surface by either: 1) regenerating the OpenAPI definition and
all public SDKs/serializers (TypeScript AlertPublicAlertType and any core.enum
declarations) so they include "feishu", or 2) make enum deserialization
backward-compatible by adding an explicit UNKNOWN/UNRECOGNIZED enum constant and a
JsonCreator/custom deserializer that maps unknown string values to UNKNOWN (and keep the
JsonValue for known values). Also add a short compatibility note to the API changelog
and add a unit/integration test that verifies clients can handle an incoming alert_type:
"feishu" without throwing.
| [ALERT_TYPE.feishu]: [ | ||
| { | ||
| sourceField: "name", | ||
| targetPath: "card.header.title.content", | ||
| }, | ||
| ], |
There was a problem hiding this comment.
ALERT_FIELD_MAPPINGS maps name to card.header.title.content for Feishu, overwriting the backend 'Opik Alert: ' prefix — should we remove that mapping for FEISHU or add the same prefix here?
Finding type: Type Inconsistency | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In apps/opik-frontend/src/v1/pages/AlertsPage/AddEditAlertPage/helpers.ts around lines
369-374, the ALERT_FIELD_MAPPINGS entry for ALERT_TYPE.feishu sets sourceField "name" ->
targetPath "card.header.title.content", which will overwrite the backend's prefixed
header ("Opik Alert: "+name). Remove this feishu mapping (or if you intentionally want
the UI example to include the prefix, change the mapping to prepend "Opik Alert: " to
the source) so the frontend example matches the backend payload. After making the
change, review
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/events/webhooks/feishu/FeishuWebhookPayloadMapper.java
around the header.title builder to confirm the backend behavior is preserved.
aadereiko
left a comment
There was a problem hiding this comment.
Hey, thanks for your contribution!
The frontend code looks good, but could you please share some videos demonstrating your changes? It would help us review everything more thoroughly :)
Thanks again!
rusherman
left a comment
There was a problem hiding this comment.
Hi @aadereiko, thanks for the review!
Here's a video demo showing the Feishu alert integration in action:
🎬 Demo Video: https://www.youtube.com/watch?v=yE7wB1xJXLA
The video covers:
- Selecting Feishu as an alert destination in the UI
- Configuring the Feishu webhook URL
- Alert cards delivered to Feishu group chat with color-coded headers and "View in Opik" action buttons
Let me know if you need anything else!
Details
Add Feishu/Lark (飞书) as the fourth native alert type alongside General, Slack, and PagerDuty.
Uses Feishu Interactive Card format for rich alert display in group chats via custom bot webhooks, covering all 10 alert event types with color-coded cards and "View in Opik" action buttons.
Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
Commands run:
Result: 36 tests run, 0 failures, 0 errors, 0 skipped
Scenarios validated:
Environment: macOS, Java 25, Maven, local process mode
Tests not run: Frontend build (requires full node_modules setup), E2E integration tests (require Docker infrastructure)
Documentation
No documentation update needed for this PR. Feishu alert type will be auto-discovered by existing UI and API patterns.