Repository navigation
fix(api): prevent Slack notification failures when alert message exceeds 3000 chars #3473
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@hyperdx/api": patch | ||
| --- | ||
|
|
||
| fix(api): prevent Slack notification failures when alert message exceeds 3000 chars |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| import { | ||
| formatSlackSectionText, | ||
| getFallbackHdxLink, | ||
| handleSendSlackWebhook, | ||
| } from '@/tasks/checkAlerts/transports/slack'; | ||
| import type { Message, WebhookChannel } from '@/tasks/checkAlerts/transports/types'; | ||
| import * as slack from '@/utils/slack'; | ||
|
|
||
| jest.mock('@/utils/slack'); | ||
| jest.mock('@/utils/validators'); | ||
|
|
||
| describe('Slack transport', () => { | ||
| describe('getFallbackHdxLink', () => { | ||
| it('returns origin + /alerts for valid URLs', () => { | ||
| expect( | ||
| getFallbackHdxLink('https://app.hyperdx.io/chart?config=%7B%22very%22%3A%22long%22%7D'), | ||
| ).toBe('https://app.hyperdx.io/alerts'); | ||
| }); | ||
|
|
||
| it('returns original input if not a valid URL', () => { | ||
| expect(getFallbackHdxLink('not-a-url')).toBe('not-a-url'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('formatSlackSectionText', () => { | ||
| it('keeps standard link and body when under character limit', () => { | ||
| const link = 'https://app.hyperdx.io/alerts/123'; | ||
| const title = 'High Error Rate'; | ||
| const body = 'Error rate exceeded 5%'; | ||
| expect(formatSlackSectionText(link, title, body)).toBe( | ||
| `*<${link} | ${title}>*\n${body}`, | ||
| ); | ||
| }); | ||
|
|
||
| it('falls back to /alerts when large query string exceeds limit', () => { | ||
| const longQuery = 'x'.repeat(3100); | ||
| const longLink = `https://app.hyperdx.io/chart?config=${longQuery}`; | ||
| const title = 'High Latency'; | ||
| const body = 'p99 latency > 500ms'; | ||
| const formatted = formatSlackSectionText(longLink, title, body, 3000); | ||
|
|
||
| expect(formatted).toBe(`*<https://app.hyperdx.io/alerts | ${title}>*\n${body}`); | ||
| expect(formatted.length).toBeLessThanOrEqual(3000); | ||
| }); | ||
|
|
||
| it('truncates body when text still exceeds limit after fallback link', () => { | ||
| const longQuery = 'x'.repeat(2900); | ||
| const longLink = `https://app.hyperdx.io/chart?config=${longQuery}`; | ||
| const title = 'High Latency'; | ||
| const longBody = 'b'.repeat(3500); | ||
| const formatted = formatSlackSectionText(longLink, title, longBody, 3000); | ||
|
|
||
| expect(formatted.startsWith(`*<https://app.hyperdx.io/alerts | ${title}>*\n`)).toBe(true); | ||
| expect(formatted.endsWith('...')).toBe(true); | ||
| expect(formatted.length).toBeLessThanOrEqual(3000); | ||
| }); | ||
| }); | ||
|
|
||
| describe('handleSendSlackWebhook', () => { | ||
| it('passes formatted section block text to postMessageToWebhook', async () => { | ||
| const mockPostMessage = jest.spyOn(slack, 'postMessageToWebhook').mockResolvedValue({} as any); | ||
|
|
||
| const channel: WebhookChannel = { | ||
| type: 'webhook', | ||
| channel: { | ||
| _id: 'wh-1' as any, | ||
| name: 'Slack Alerts', | ||
|
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. 🔵 minor — The test uses Build the fixtures with real types ( Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond |
||
| service: 'slack' as any, | ||
| url: 'https://hooks.slack.com/services/T00/B00/X00', | ||
| } as any, | ||
|
Comment on lines
+65
to
+70
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.
Casting the entire channel fixture to Context Used: AGENTS.md (source) Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| }; | ||
|
|
||
| const message: Message = { | ||
| hdxLink: 'https://app.hyperdx.io/chart?config=test', | ||
| title: 'CPU Alert', | ||
| body: 'CPU > 90%', | ||
| state: 'ALERT' as any, | ||
| startTime: 0, | ||
| endTime: 1000, | ||
| eventId: 'evt-1', | ||
| }; | ||
|
|
||
| await handleSendSlackWebhook(channel, message); | ||
|
|
||
| expect(mockPostMessage).toHaveBeenCalledTimes(1); | ||
| expect(mockPostMessage).toHaveBeenCalledWith( | ||
| 'https://hooks.slack.com/services/T00/B00/X00', | ||
| { | ||
| text: 'CPU Alert', | ||
| blocks: [ | ||
| { | ||
|
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. 🔵 minor — Webhook test never exercises the over-limit path the PR fixes
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond |
||
| type: 'section', | ||
| text: { | ||
| type: 'mrkdwn', | ||
| text: '*<https://app.hyperdx.io/chart?config=test | CPU Alert>*\nCPU > 90%', | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,49 @@ import type { | |
| import * as slack from '@/utils/slack'; | ||
| import { validateWebhookUrl } from '@/utils/validators'; | ||
|
|
||
| const MAX_SLACK_SECTION_LENGTH = 3000; | ||
|
|
||
| export const getFallbackHdxLink = (hdxLink: string): string => { | ||
|
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. 🔵 minor — Transport guesses app routes by string-parsing hdxLink; the fallback belongs in the provider and should target the specific alert The Slack transport has no knowledge of frontend routes or the alert id, so all it can do is link to the generic list. Have Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond |
||
| try { | ||
| const url = new URL(hdxLink); | ||
| return `${url.origin}/alerts`; | ||
|
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. 🟠 major — Fallback link uses Build the fallback from Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
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. This is a valid concern. @Tyagiquamar Could you address it?
Comment on lines
+20
to
+21
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.
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. 🟠 major — Fallback link drops the deployment path prefix and points to a page that doesn't exist The links come from Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond |
||
| } catch { | ||
| return hdxLink; | ||
| } | ||
| }; | ||
|
|
||
| export const formatSlackSectionText = ( | ||
| hdxLink: string, | ||
| title: string, | ||
| body: string, | ||
| maxLength = MAX_SLACK_SECTION_LENGTH, | ||
| ): string => { | ||
| let link = hdxLink; | ||
| let text = `*<${link} | ${title}>*\n${body}`; | ||
| if (text.length <= maxLength) { | ||
| return text; | ||
| } | ||
|
|
||
| // If text exceeds limit, try falling back to the shorter /alerts link | ||
| const fallbackLink = getFallbackHdxLink(hdxLink); | ||
|
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. 🟠 major — The deep link is swapped for generic Only fall back when the link itself is the problem: if Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond |
||
| if (fallbackLink !== link) { | ||
| text = `*<${fallbackLink} | ${title}>*\n${body}`; | ||
| if (text.length <= maxLength) { | ||
| return text; | ||
| } | ||
| link = fallbackLink; | ||
| } | ||
|
|
||
| // If still exceeding limit, truncate body with an ellipsis | ||
| const header = `*<${link} | ${title}>*\n`; | ||
| const availableBodyLen = maxLength - header.length - 3; | ||
| if (availableBodyLen > 0) { | ||
| return `${header}${body.slice(0, availableBodyLen)}...`; | ||
|
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. 🔵 minor — Body truncation cuts off the closing ``` of the saved-search results block Saved-search bodies end with a Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
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. 🔵 minor — Hand-written slice-plus-ellipsis duplicates the existing Use Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
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. 🔵 minor — Re-implements the existing truncateString helper
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond |
||
| } | ||
|
|
||
| return `*<${link} | ${title}>*`.slice(0, maxLength); | ||
|
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. 🔵 minor — Last-resort branch cuts through the mrkdwn link syntax Slicing Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
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.
When the title makes the header exceed 3000 characters, this branch cuts through the Slack link text and removes its closing
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. 🔵 minor — Last-resort slice cuts through the mrkdwn link markup When the title alone pushes the header past the limit, Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond |
||
| }; | ||
|
|
||
| export const handleSendSlackWebhook = async ( | ||
| channel: WebhookChannel, | ||
| message: Message, | ||
|
|
@@ -29,7 +72,11 @@ export const handleSendSlackWebhook = async ( | |
| type: 'section', | ||
| text: { | ||
| type: 'mrkdwn', | ||
| text: `*<${message.hdxLink} | ${message.title}>*\n${message.body}`, | ||
| text: formatSlackSectionText( | ||
| message.hdxLink, | ||
| message.title, | ||
| message.body, | ||
| ), | ||
|
Comment on lines
+75
to
+79
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. This changes notification behavior in Context Used: AGENTS.md (source) Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| }, | ||
| }, | ||
| ], | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 minor — The webhook integration test only covers the short-message path, which already worked
Add a
handleSendSlackWebhookcase with a >3000-charhdxLink(like the real inline-alert/chart?config=link) and assert the posted block text is ≤3000 chars and uses the fallback link. Also add a case for aFRONTEND_URLthat has a path prefix. Without these, removing theformatSlackSectionTextcall from the send path would still pass.Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond