diff --git a/apps/pg-sync-service/src/lib/db.ts b/apps/pg-sync-service/src/lib/db.ts index 53dfd1d9b..536b8cc86 100644 --- a/apps/pg-sync-service/src/lib/db.ts +++ b/apps/pg-sync-service/src/lib/db.ts @@ -1,5 +1,6 @@ -import { PgAirtableDb, createTestDbClients } from '@bluedot/db'; +import { PgAirtableDb, createTestDbClients, formatAirtableWarning } from '@bluedot/db'; import { slackAlert } from '@bluedot/utils'; +import { logger } from '@bluedot/ui/src/api'; import env from '../env'; const isTest = env.VITEST === 'true'; @@ -17,20 +18,16 @@ export const db = new PgAirtableDb({ airtableApiKey: env.AIRTABLE_PERSONAL_ACCESS_TOKEN, ...(isTest ? createTestDbClients() : {}), onWarning: async (warning: unknown) => { - const err = warning instanceof Error ? warning : new Error(String(warning)); - const message = `Airtable validation warning encountered, attempting to proceed by setting the affected fields to undefined. Warning message: ${err.message}`; + const { message, messages, batchGroup } = formatAirtableWarning(warning); - // eslint-disable-next-line no-console - console.warn(message); + logger.warn(message); - await slackAlert(env, [ - message, - ...(err.stack ? [`Stack:\n\`\`\`${err.stack}\`\`\``] : []), - ], { + await slackAlert(env, messages, { channelId: env.PG_SYNC_SLACK_CHANNEL_ID, batchKey: 'airtable-validation', spikeThreshold: VALIDATION_WARNING_SPIKE_THRESHOLD, escalationChannelId: env.ALERTS_SLACK_CHANNEL_ID, + batchGroup, }); }, }); diff --git a/apps/website/src/lib/api/db/index.ts b/apps/website/src/lib/api/db/index.ts index 34b4803c4..784d91764 100644 --- a/apps/website/src/lib/api/db/index.ts +++ b/apps/website/src/lib/api/db/index.ts @@ -1,5 +1,6 @@ -import { PgAirtableDb, createTestDbClients } from '@bluedot/db'; +import { PgAirtableDb, createTestDbClients, formatAirtableWarning } from '@bluedot/db'; import { slackAlert } from '@bluedot/utils'; +import { logger } from '@bluedot/ui/src/api'; import env from '../env'; const isTest = env.VITEST === 'true'; @@ -9,10 +10,9 @@ export default new PgAirtableDb({ airtableApiKey: env.AIRTABLE_PERSONAL_ACCESS_TOKEN, ...(isTest ? createTestDbClients() : {}), async onWarning(warning: unknown) { - const err = warning instanceof Error ? warning : new Error(String(warning)); - await slackAlert(env, [ - `Airtable validation warning encountered, attempting to proceed by setting the affected fields to undefined. Warning message: ${err.message}`, - ...(err.stack ? [`Stack:\n\`\`\`${err.stack}\`\`\``] : []), - ], { batchKey: 'airtable-validation' }); + const { message, messages, batchGroup } = formatAirtableWarning(warning); + + logger.warn(message); + await slackAlert(env, messages, { batchKey: 'airtable-validation', batchGroup }); }, }); diff --git a/libraries/db/src/index.ts b/libraries/db/src/index.ts index 5ef73ef7a..a8bd12f01 100644 --- a/libraries/db/src/index.ts +++ b/libraries/db/src/index.ts @@ -117,5 +117,8 @@ export type { DeprecationSafeTable } from './lib/db-core'; export { AirtableTsError, ErrorType } from 'airtable-ts/dist/AirtableTsError'; +export { formatAirtableWarning } from './lib/formatAirtableWarning'; +export type { FormattedAirtableWarning } from './lib/formatAirtableWarning'; + // TODO: restrict what's exported export * from 'drizzle-orm'; diff --git a/libraries/db/src/lib/db-core.ts b/libraries/db/src/lib/db-core.ts index 7ec72a74b..f82d8c3c7 100644 --- a/libraries/db/src/lib/db-core.ts +++ b/libraries/db/src/lib/db-core.ts @@ -224,6 +224,15 @@ export function getPgAirtableFromIds({ baseId, tableId }: { baseId: string; tabl return pgAirtableTableRegistry[key]; } +/** + * Airtable table ids are unique across bases, so a table id alone is enough to recover + * its base id. Useful when all we have is an error message from airtable-ts, which + * mentions the table id but not the base. + */ +export function getPgAirtableFromTableId(tableId: string) { + return Object.values(pgAirtableTableRegistry).find((table) => table.airtable.tableId === tableId); +} + export function pgAirtable< TTableName extends string, TColumnsMap extends Record, diff --git a/libraries/db/src/lib/formatAirtableWarning.test.ts b/libraries/db/src/lib/formatAirtableWarning.test.ts new file mode 100644 index 000000000..6506101aa --- /dev/null +++ b/libraries/db/src/lib/formatAirtableWarning.test.ts @@ -0,0 +1,105 @@ +import { describe, expect, test } from 'vitest'; +import { mapRecordFromAirtable } from 'airtable-ts/dist/mapping/recordMapper'; +import type { AirtableRecord } from 'airtable-ts/dist/types'; +import { formatAirtableWarning } from './formatAirtableWarning'; +import { selfServeCourseRegistrationTable } from '../schema'; + +// A real schema entry, so the table id resolves to a base id and a column name. +const TABLE_ID = 'tbla338CpAd0FF96g'; +const BASE_ID = 'appnJbsG1eWbAdEvf'; +const FIELD_ID = 'fldsS2lCVlk1WDSDw'; +const RECORD_ID = 'rec1ArmjQ8wUmBwDC'; +const RECORD_LINK = ``; + +describe('formatAirtableWarning', () => { + const typeMismatch = `Failed to map record from Airtable format for table 'self_serve_course_registration' (${TABLE_ID}) and record ${RECORD_ID}: Failed to map field fullName (${FIELD_ID}) from Airtable: Cannot convert value from airtable type 'multipleLookupValues' to 'string | null', as the Airtable API provided a 'object'. Suggestion: Update the types in your table definition to compatible types for your Airtable base.`; + + test('reduces a field-level warning to concise prose and batch metadata', () => { + const formatted = formatAirtableWarning(new Error(typeMismatch)); + + expect(formatted.message).toBe(`Field \`fullName\` on \`self_serve_course_registration\` (record ${RECORD_ID}): can't map Airtable multipleLookupValues → string | null (got object). Value defaulted.`); + expect(formatted.batchGroup.signature).toBe(`${TABLE_ID}/${FIELD_ID}`); + }); + + test('links affected records into Airtable, using the base id from our schema', () => { + const formatted = formatAirtableWarning(new Error(typeMismatch)); + + // Only the Slack-bound copy carries mrkdwn; `message` stays plain for logs. + expect(formatted.messages[0]).toContain(RECORD_LINK); + expect(formatted.batchGroup.dedupeKeys).toEqual([RECORD_LINK]); + }); + + test('includes the stack trace as a reply message', () => { + const err = new Error(typeMismatch); + const formatted = formatAirtableWarning(err); + + expect(formatted.messages).toHaveLength(2); + expect(formatted.messages[1]).toBe(`Stack:\n\`\`\`${err.stack}\`\`\``); + }); + + test('names the field from our schema when the warning carries no field prefix', () => { + // airtable-ts reports a field deleted from Airtable without its usual + // "Failed to map field ..." prefix, so the field name has to come from our schema. + const formatted = formatAirtableWarning(new Error(`Failed to map record from Airtable format for table 'self_serve_course_registration' (${TABLE_ID}) and record ${RECORD_ID}: Field '${FIELD_ID}' does not exist in the table definition. This error should not happen in normal operation.`)); + + expect(formatted.message).toBe(`Field \`fullName\` on \`self_serve_course_registration\` (record ${RECORD_ID}): Field '${FIELD_ID}' does not exist in the table definition. This error should not happen in normal operation. Value defaulted.`); + expect(formatted.batchGroup.signature).toBe(`${TABLE_ID}/${FIELD_ID}`); + }); + + test('formats a table we do not own, minus the record link', () => { + const formatted = formatAirtableWarning(new Error('Failed to map record from Airtable format for table \'someone_elses_table\' (tblXXXXXXXXXXXXXX) and record recYYYYYYYYYYYYYY: Failed to map field someField (fldZZZZZZZZZZZZZZ) from Airtable: Cannot convert value from airtable type \'formula\' to \'number\', as the Airtable API provided a \'string\'.')); + + expect(formatted.message).toBe('Field `someField` on `someone_elses_table` (record recYYYYYYYYYYYYYY): can\'t map Airtable formula → number (got string). Value defaulted.'); + // No base id available, so no link. + expect(formatted.batchGroup.dedupeKeys).toEqual(['recYYYYYYYYYYYYYY']); + }); + + test('keys record-level failures on the reason, with ids scrubbed', () => { + // Groups the same failure across records, without merging unrelated failures on one table. + const formatted = formatAirtableWarning(new Error(`Failed to map record from Airtable format for table 'self_serve_course_registration' (${TABLE_ID}) and record ${RECORD_ID}: something unexpected happened about ${RECORD_ID}`)); + + expect(formatted.message).toBe(`Record ${RECORD_ID} on \`self_serve_course_registration\`: something unexpected happened about ${RECORD_ID}.`); + expect(formatted.batchGroup.signature).toBe(`${TABLE_ID}/something unexpected happened about rec***`); + }); + + test('falls back to the raw message with no batch metadata when the shape is absent', () => { + const formatted = formatAirtableWarning(new Error('Some unrelated error with no airtable ids')); + + expect(formatted.message).toBe('Some unrelated error with no airtable ids'); + expect(formatted.batchGroup).toEqual({}); + }); + + test('coerces non-Error warnings', () => { + const formatted = formatAirtableWarning('string warning'); + + expect(formatted.message).toBe('string warning'); + expect(formatted.batchGroup).toEqual({}); + }); + + test('parses what airtable-ts actually emits, not just our fixture strings', async () => { + // Guards the prose regexes against an airtable-ts wording change: without this, a + // version bump could silently degrade us to the ungrouped fallback with CI still green. + const record = { + id: RECORD_ID, + fields: { fullName: {} }, + _table: { fields: [{ id: FIELD_ID, name: 'fullName', type: 'multipleLookupValues' }] }, + } as unknown as AirtableRecord; + + const warnings: unknown[] = []; + mapRecordFromAirtable(selfServeCourseRegistrationTable.airtable, record, { + readValidation: 'warning', + onWarning: (warning) => { + warnings.push(warning); + }, + }); + // onWarning is dispatched in a floating promise, so let the microtask queue drain. + await Promise.resolve(); + + const warning = warnings.find((w): w is Error => w instanceof Error && w.message.includes(FIELD_ID)); + expect(warning).toBeDefined(); + + const formatted = formatAirtableWarning(warning); + expect(formatted.message).toBe(`Field \`fullName\` on \`self_serve_course_registration\` (record ${RECORD_ID}): can't map Airtable multipleLookupValues → string | null (got object). Value defaulted.`); + expect(formatted.batchGroup.signature).toBe(`${TABLE_ID}/${FIELD_ID}`); + }); +}); diff --git a/libraries/db/src/lib/formatAirtableWarning.ts b/libraries/db/src/lib/formatAirtableWarning.ts new file mode 100644 index 000000000..09588a696 --- /dev/null +++ b/libraries/db/src/lib/formatAirtableWarning.ts @@ -0,0 +1,98 @@ +import { type BatchGroup } from '@bluedot/utils'; +import { getPgAirtableFromTableId } from './db-core'; + +/** + * airtable-ts surfaces read-validation failures as one deeply-nested error string, e.g.: + * Failed to map record from Airtable format for table 'self_serve_course_registration' + * (tbl…) and record rec…: Failed to map field fullName (fld…) from Airtable: Cannot + * convert value from airtable type 'multipleLookupValues' to 'string | null', as the + * Airtable API provided a 'object'. Suggestion: Update the types... + * + * We peel off the prefixes airtable-ts prepends as the error bubbles up, keeping the innermost + * reason plus the ids. Our own table definitions are consulted only for what the message can't + * tell us: the base id needed to link the offending record, and the column name in the cases + * where airtable-ts reports a field by id alone. + */ +export type FormattedAirtableWarning = { + /** Plain text, for logs. */ + message: string; + /** Slack-flavoured (record ids become links), plus a stack-trace reply. Pass straight to slackAlert. */ + messages: string[]; + batchGroup: BatchGroup; +}; + +const TABLE_ID = /\btbl[A-Za-z0-9]{10,}/; +const FIELD_ID = /\bfld[A-Za-z0-9]{10,}/; +const RECORD_ID = /\brec[A-Za-z0-9]{10,}/; + +// Prefixes airtable-ts prepends as an error bubbles up, plus the suggestion it appends. +// Stripping them leaves the innermost reason, which we can quote without repeating the +// location we've already stated. +const RECORD_PREFIX = /^Failed to map record from Airtable format for table '([^']*)' \(tbl[A-Za-z0-9]{10,}\) and record rec[A-Za-z0-9]{10,}: /; +const FIELD_PREFIX = /^Failed to map field (.+?)(?: \(fld[A-Za-z0-9]{10,}\))? from Airtable: /; +const SUGGESTION = / Suggestion: [\s\S]*$/; +const TYPE_MISMATCH = /from airtable type '([^']*)' to '([^']*)', as the Airtable API provided a '([^']*)'/; + +const scrubIds = (message: string) => message.replace(/\b(tbl|fld|rec)[A-Za-z0-9]{10,}/g, '$1***'); + +export const formatAirtableWarning = (warning: unknown): FormattedAirtableWarning => { + const err = warning instanceof Error ? warning : new Error(String(warning)); + const raw = err.message; + const withStack = (message: string) => [message, ...(err.stack ? [`Stack:\n\`\`\`${err.stack}\`\`\``] : [])]; + + const tableId = TABLE_ID.exec(raw)?.[0]; + const recordId = RECORD_ID.exec(raw)?.[0]; + if (!tableId || !recordId) { + return { message: raw, messages: withStack(raw), batchGroup: {} }; + } + + // Peel the prefixes off one at a time, capturing what each one carries as we go. + const recordPrefix = RECORD_PREFIX.exec(raw); + const afterRecord = recordPrefix ? raw.slice(recordPrefix[0].length) : raw; + const fieldPrefix = FIELD_PREFIX.exec(afterRecord); + const afterField = fieldPrefix ? afterRecord.slice(fieldPrefix[0].length) : afterRecord; + const innermost = afterField.replace(SUGGESTION, ''); + + const fieldId = FIELD_ID.exec(raw)?.[0]; + const table = getPgAirtableFromTableId(tableId); + const columnName = fieldId + ? Array.from(table?.airtableFieldMap ?? []).find(([, airtableId]) => airtableId === fieldId)?.[0] + : undefined; + + const tableName = recordPrefix?.[1] ?? tableId; + // airtable-ts usually names the field in its prefix. When it doesn't — e.g. a field deleted + // from Airtable, reported by id — recover the column name rather than print a bare id. + const fieldName = fieldPrefix?.[1] ?? columnName ?? fieldId; + + const [, airtableType, expectedType, providedType] = TYPE_MISMATCH.exec(innermost) ?? []; + const reason = airtableType && expectedType + ? `can't map Airtable ${airtableType} → ${expectedType}${providedType ? ` (got ${providedType})` : ''}` + : innermost; + + const buildMessage = (recordRef: string) => { + const location = fieldId + ? `Field \`${fieldName}\` on \`${tableName}\` (record ${recordRef})` + : `Record ${recordRef} on \`${tableName}\``; + // airtable-ts substitutes a type-appropriate default (null, '', 0, false or []), so + // don't promise a specific value here. + const outcome = fieldId ? ' Value defaulted.' : ''; + return `${location}: ${reason.replace(/\.$/, '')}.${outcome}`; + }; + + const baseId = table?.airtable.baseId; + const recordLink = baseId + ? `` + : recordId; + + return { + message: buildMessage(recordId), + messages: withStack(buildMessage(recordLink)), + batchGroup: { + // Group by table/field, not by record, so the same failure across many records + // collapses into one batched alert. With no field to key on, fall back to the + // reason with ids scrubbed, so unrelated failures on one table stay separate. + signature: `${tableId}/${fieldId ?? scrubIds(innermost)}`, + dedupeKeys: [recordLink], + }, + }; +}; diff --git a/libraries/utils/src/index.ts b/libraries/utils/src/index.ts index f43322ab3..4d57410dc 100644 --- a/libraries/utils/src/index.ts +++ b/libraries/utils/src/index.ts @@ -1,5 +1,5 @@ export { validateEnv } from './validateEnv'; -export { slackAlert } from './slackNotifications'; +export { slackAlert, type BatchGroup } from './slackNotifications'; export { chunk } from './array'; export { diff --git a/libraries/utils/src/slackNotifications.test.ts b/libraries/utils/src/slackNotifications.test.ts index 6f7a46914..2bd2c6bee 100644 --- a/libraries/utils/src/slackNotifications.test.ts +++ b/libraries/utils/src/slackNotifications.test.ts @@ -124,15 +124,16 @@ describe('slackNotifications', () => { }); describe('batching mode', () => { - test('should batch similar messages by signature', async () => { - const message1 = 'Failed to map field unitNumber (fldL42M2hgchJYIdD) from Airtable for table exercise (tbla7lc2MtSSbWVvS) and record rec3BGObwkLPSskvb'; - const message2 = 'Failed to map field unitNumber (fldL42M2hgchJYIdD) from Airtable for table exercise (tbla7lc2MtSSbWVvS) and record rec3qTvZcFGYccFcl'; - const message3 = 'Failed to map field unitNumber (fldL42M2hgchJYIdD) from Airtable for table exercise (tbla7lc2MtSSbWVvS) and record rec3060zKybkbm9UH'; + test('should batch by shared signature and render dedupeKeys', async () => { + const message = 'Field `unitNumber` on `exercise`: cannot map value. Set to undefined.'; + const batchGroupFor = (recordId: string) => ({ + signature: 'exercise/unitNumber', + dedupeKeys: [recordId], + }); - // Add messages to batch - slackAlert(mockEnv, [message1], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS }); - slackAlert(mockEnv, [message2], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS }); - slackAlert(mockEnv, [message3], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS }); + slackAlert(mockEnv, [message], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, batchGroup: batchGroupFor('rec3BGObwkLPSskvb') }); + slackAlert(mockEnv, [message], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, batchGroup: batchGroupFor('rec3qTvZcFGYccFcl') }); + slackAlert(mockEnv, [message], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, batchGroup: batchGroupFor('rec3060zKybkbm9UH') }); // No messages sent yet expect(fetchMock).not.toHaveBeenCalled(); @@ -151,12 +152,10 @@ describe('slackNotifications', () => { const callBody = JSON.parse(fetchMock.mock.calls[0]?.[1].body); expect(callBody.text).toContain('test-app:'); - expect(callBody.text).toContain('This error occurred 3 times affecting 3 record(s)'); + expect(callBody.text).toContain('This error occurred 3 times affecting 3 item(s)'); expect(callBody.text).toContain('rec3BGObwkLPSskvb'); expect(callBody.text).toContain('rec3qTvZcFGYccFcl'); expect(callBody.text).toContain('rec3060zKybkbm9UH'); - expect(callBody.text).toContain('Table: tbla7lc2MtSSbWVvS'); - expect(callBody.text).toContain('Field: fldL42M2hgchJYIdD'); }); test('should send single occurrence without batching summary', async () => { @@ -200,9 +199,32 @@ describe('slackNotifications', () => { expect(callBody.text).not.toContain('affecting'); }); + test('should collapse differing messages that share an explicit signature', async () => { + // Distinct message bodies but the same signature must land in one batch. + slackAlert(mockEnv, ['Error affecting Alice'], { + batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, batchGroup: { signature: 'same-group', dedupeKeys: ['a'] }, + }); + slackAlert(mockEnv, ['Error affecting Bob'], { + batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, batchGroup: { signature: 'same-group', dedupeKeys: ['b'] }, + }); + + fetchMock.mockResolvedValueOnce({ + ok: true, + json: async () => ({ ok: true, ts: '1.0' }), + }); + + await vi.advanceTimersByTimeAsync(DEFAULT_FLUSH_INTERVAL_MS); + + expect(fetchMock).toHaveBeenCalledTimes(1); + const callBody = JSON.parse(fetchMock.mock.calls[0]?.[1].body); + // First message wins as the batch's main message. + expect(callBody.text).toContain('Error affecting Alice'); + expect(callBody.text).toContain('This error occurred 2 times affecting 2 item(s)'); + }); + test('should preserve first reply in batched messages', async () => { - const message1 = ['Main error message rec1AbCdEfGhIjKl', 'Stack trace here']; - const message2 = ['Main error message rec2MnOpQrStUvWx', 'Different stack trace']; + const message1 = ['Main error message', 'Stack trace here']; + const message2 = ['Main error message', 'Different stack trace']; slackAlert(mockEnv, message1, { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS }); slackAlert(mockEnv, message2, { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS }); @@ -264,14 +286,12 @@ describe('slackNotifications', () => { expect(reply2Body.text).toBe('test-app: Reply 2'); }); - test('should track unique record IDs across batches', async () => { - const message1 = 'Error rec1AbCdEfGhIjKl'; - const message2 = 'Error rec1AbCdEfGhIjKl'; // Same record - const message3 = 'Error rec2MnOpQrStUvWx'; + test('should deduplicate items across batches', async () => { + const dedupe = (key: string) => ({ batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, batchGroup: { signature: 'shared-batch', dedupeKeys: [key] } }); - slackAlert(mockEnv, [message1], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS }); - slackAlert(mockEnv, [message2], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS }); - slackAlert(mockEnv, [message3], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS }); + slackAlert(mockEnv, ['Error'], dedupe('rec1AbCdEfGhIjKl')); + slackAlert(mockEnv, ['Error'], dedupe('rec1AbCdEfGhIjKl')); // Same key + slackAlert(mockEnv, ['Error'], dedupe('rec2MnOpQrStUvWx')); fetchMock.mockResolvedValueOnce({ ok: true, @@ -281,16 +301,16 @@ describe('slackNotifications', () => { await vi.advanceTimersByTimeAsync(DEFAULT_FLUSH_INTERVAL_MS); const callBody = JSON.parse(fetchMock.mock.calls[0]?.[1].body); - expect(callBody.text).toContain('affecting 2 record(s)'); // Only unique records + expect(callBody.text).toContain('affecting 2 item(s)'); // Only unique keys expect(callBody.text).toContain('rec1AbCdEfGhIjKl'); expect(callBody.text).toContain('rec2MnOpQrStUvWx'); }); - test('should limit record list to 10 records', async () => { + test('should limit the item list to 10', async () => { const recordIds = Array.from({ length: 15 }, (_, i) => `rec${i}AbCdEfGhIjKl`); for (const recordId of recordIds) { - slackAlert(mockEnv, [`Error ${recordId}`], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS }); + slackAlert(mockEnv, ['Error'], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, batchGroup: { signature: 'shared-batch', dedupeKeys: [recordId] } }); } fetchMock.mockResolvedValueOnce({ @@ -301,7 +321,7 @@ describe('slackNotifications', () => { await vi.advanceTimersByTimeAsync(DEFAULT_FLUSH_INTERVAL_MS); const callBody = JSON.parse(fetchMock.mock.calls[0]?.[1].body); - expect(callBody.text).toContain('affecting 15 record(s)'); + expect(callBody.text).toContain('affecting 15 item(s)'); expect(callBody.text).toContain('and 5 more'); // 15 - 10 = 5 }); @@ -395,16 +415,16 @@ describe('slackNotifications', () => { describe('spike escalation', () => { const escalationChannelId = 'alerts-channel'; - test('cross-posts batched summary to escalation channel when record count hits threshold', async () => { - const messages = ['rec1AbCdEfGhIjKl', 'rec2MnOpQrStUvWx', 'rec3ZzYyXxWwVvUu'] - .map((recordId) => `Validation warning ${recordId}`); + test('cross-posts batched summary to escalation channel when item count hits threshold', async () => { + const recordIds = ['rec1AbCdEfGhIjKl', 'rec2MnOpQrStUvWx', 'rec3ZzYyXxWwVvUu']; - for (const message of messages) { - slackAlert(mockEnv, [message], { + for (const recordId of recordIds) { + slackAlert(mockEnv, ['Validation warning'], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, spikeThreshold: 3, escalationChannelId, + batchGroup: { signature: 'shared-batch', dedupeKeys: [recordId] }, }); } @@ -444,16 +464,16 @@ describe('slackNotifications', () => { }); test('does not escalate when escalation channel equals batch channel', async () => { - const messages = ['rec1AbCdEfGhIjKl', 'rec2MnOpQrStUvWx', 'rec3ZzYyXxWwVvUu'] - .map((recordId) => `Validation warning ${recordId}`); + const recordIds = ['rec1AbCdEfGhIjKl', 'rec2MnOpQrStUvWx', 'rec3ZzYyXxWwVvUu']; - for (const message of messages) { - slackAlert(mockEnv, [message], { + for (const recordId of recordIds) { + slackAlert(mockEnv, ['Validation warning'], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, spikeThreshold: 3, channelId: 'test-channel', escalationChannelId: 'test-channel', + batchGroup: { signature: 'shared-batch', dedupeKeys: [recordId] }, }); } @@ -465,7 +485,7 @@ describe('slackNotifications', () => { }); test('falls back to occurrence count when messages carry no record IDs', async () => { - // No rec IDs -> affectedRecords stays empty -> spikeCount uses raw occurrences. + // No dedupeKeys -> dedupedItems stays empty -> spikeCount uses raw occurrences. for (let i = 0; i < 3; i++) { slackAlert(mockEnv, ['Validation warning with no record id'], { batchKey: 'test', @@ -487,18 +507,24 @@ describe('slackNotifications', () => { expect(escalationBody.text).toContain('3 affected'); }); - test('uses spike options from a later call in the same window', async () => { - // First call omits spike options; a later call in the same window supplies them. - // The later options should take effect rather than being silently ignored. - slackAlert(mockEnv, ['Validation warning rec1AbCdEfGhIjKl'], { + // Spike options are per-call but apply to the whole batcher, so whichever call in the + // window carries them must win: a later call must not be ignored, and a later call + // that omits them must not clear them. + test.each([ + ['a later call supplies them', [{}, { spikeThreshold: 2, escalationChannelId }]], + ['a later call omits them', [{ spikeThreshold: 2, escalationChannelId }, {}]], + ] as const)('escalates when %s', async (_name, [firstOptions, secondOptions]) => { + slackAlert(mockEnv, ['Validation warning'], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, + batchGroup: { signature: 'shared-batch', dedupeKeys: ['rec1AbCdEfGhIjKl'] }, + ...firstOptions, }); - slackAlert(mockEnv, ['Validation warning rec2MnOpQrStUvWx'], { + slackAlert(mockEnv, ['Validation warning'], { batchKey: 'test', flushIntervalMs: DEFAULT_FLUSH_INTERVAL_MS, - spikeThreshold: 2, - escalationChannelId, + batchGroup: { signature: 'shared-batch', dedupeKeys: ['rec2MnOpQrStUvWx'] }, + ...secondOptions, }); fetchMock diff --git a/libraries/utils/src/slackNotifications.ts b/libraries/utils/src/slackNotifications.ts index b2b29b44e..d8a3f9ed7 100644 --- a/libraries/utils/src/slackNotifications.ts +++ b/libraries/utils/src/slackNotifications.ts @@ -4,25 +4,37 @@ type SlackAlertEnv = { ALERTS_SLACK_CHANNEL_ID: string; }; +// Domain-neutral batching metadata. Callers describe how to group and summarise their +// messages without the util needing to know anything about the message contents. +export type BatchGroup = { + // Grouping key: messages sharing a signature collapse into one batch. Defaults to the + // message text, so callers wanting to group by e.g. error type (ignoring embedded IDs) + // must supply a stable signature themselves. + signature?: string; + // Distinct affected items. Deduplicated across the window, counted, and listed in the + // batched summary ("affecting N item(s)", first 10 + "and M more"). + dedupeKeys?: string[]; +}; + type SlackAlertOptions = { channelId?: string; batchKey?: string; flushIntervalMs?: number; - // When a batched signature affects at least this many distinct records within a + // When a batched signature affects at least this many distinct items within a // flush window, cross-post the batched summary to escalationChannelId. Lets a // low-priority side channel stay quiet for routine drift while a sudden base-wide // spike still reaches the main alerts channel. spikeThreshold?: number; escalationChannelId?: string; + // How to group and summarise batched messages. Only used when batchKey is set. + batchGroup?: BatchGroup; }; type MessageBatch = { signature: string; occurrences: number; messages: string[]; - tableId: string | null; - fieldId: string | null; - affectedRecords: Set; + dedupedItems: Set; lastSeen: number; }; @@ -43,15 +55,16 @@ const batchers = new Map(); * Sends Slack message(s) to our prod/dev channels * - By default, messages are sent immediately to ALERTS_SLACK_CHANNEL_ID * - To send to a different channel, provide a channelId - * - To enable batching, provide a batchKey - messages will be grouped by signature (same error type/table/field) + * - To enable batching, provide a batchKey - messages are grouped by signature (the message text by default, or batchGroup.signature) * - Batching uses a rolling window: messages are sent N ms after the last message (not from the first) * - If multiple messages are provided, the first is sent as a new message, and the rest are sent as replies in a thread * * @param options.channelId - If provided, sends to this channel instead of ALERTS_SLACK_CHANNEL_ID * @param options.batchKey - If provided, enables batching with this key as the batch group identifier * @param options.flushIntervalMs - Time window for batching in ms (default: 60000, only used when batchKey is provided). Uses a rolling window - each new message resets the timer. - * @param options.spikeThreshold - If provided, when a batched signature affects at least this many distinct records within a flush window, cross-post the batched summary to escalationChannelId. + * @param options.spikeThreshold - If provided, when a batched signature affects at least this many distinct items within a flush window, cross-post the batched summary to escalationChannelId. * @param options.escalationChannelId - If provided, specifies the channel to which batched summaries are cross-posted when the spikeThreshold is exceeded. + * @param options.batchGroup - If provided, controls how batched messages are grouped and summarised (signature, dedupeKeys). */ export const slackAlert = async ( env: SlackAlertEnv, @@ -69,6 +82,7 @@ export const slackAlert = async ( addToBatch(getBatcherKey(options.batchKey, env, channelId), env, messages, channelId, flushIntervalMs, { spikeThreshold: options.spikeThreshold, escalationChannelId: options.escalationChannelId, + batchGroup: options.batchGroup, }); return; } @@ -95,15 +109,16 @@ const addToBatch = ( messages: string[], channelId: string, flushIntervalMs: number, - spikeOptions: { spikeThreshold?: number; escalationChannelId?: string }, + batchOptions: { spikeThreshold?: number; escalationChannelId?: string; batchGroup?: BatchGroup }, ) => { const [mainMessage] = messages; if (!mainMessage) { return; } - const { tableId, fieldId, recordIds } = extractAirtableIds(mainMessage); - const signature = getMessageSignature(mainMessage); + const { batchGroup } = batchOptions; + const signature = batchGroup?.signature ?? mainMessage; + const dedupeKeys = batchGroup?.dedupeKeys ?? []; let batcher = batchers.get(batchKey); @@ -113,32 +128,30 @@ const addToBatch = ( flushTimer: null, env, channelId, - spikeThreshold: spikeOptions.spikeThreshold, - escalationChannelId: spikeOptions.escalationChannelId, + spikeThreshold: batchOptions.spikeThreshold, + escalationChannelId: batchOptions.escalationChannelId, }; batchers.set(batchKey, batcher); } else { - // Keep spike settings current: a later call in the same window should not be - // silently ignored just because the batcher already exists. - batcher.spikeThreshold = spikeOptions.spikeThreshold; - batcher.escalationChannelId = spikeOptions.escalationChannelId; + // Keep spike settings current without letting a call that omits them clear what an + // earlier call in the same window established. + batcher.spikeThreshold = batchOptions.spikeThreshold ?? batcher.spikeThreshold; + batcher.escalationChannelId = batchOptions.escalationChannelId ?? batcher.escalationChannelId; } const existing = batcher.batches.get(signature); if (existing) { existing.occurrences += 1; existing.lastSeen = Date.now(); - recordIds.forEach((id) => { - existing.affectedRecords.add(id); + dedupeKeys.forEach((id) => { + existing.dedupedItems.add(id); }); } else { batcher.batches.set(signature, { signature, occurrences: 1, messages, - tableId, - fieldId, - affectedRecords: new Set(recordIds), + dedupedItems: new Set(dedupeKeys), lastSeen: Date.now(), }); } @@ -149,21 +162,6 @@ const addToBatch = ( }); }; -const getMessageSignature = (message: string) => { - // Match Airtable IDs (tbl/fld/rec + 10+ alphanumeric chars) - return message - .replace(/\btbl[A-Za-z0-9]{10,}/g, 'tbl***') - .replace(/\bfld[A-Za-z0-9]{10,}/g, 'fld***') - .replace(/\brec[A-Za-z0-9]{10,}/g, 'rec***'); -}; - -const extractAirtableIds = (message: string) => { - const tableId = (/\btbl[A-Za-z0-9]{10,}/.exec(message))?.[0] ?? null; - const fieldId = (/\bfld[A-Za-z0-9]{10,}/.exec(message))?.[0] ?? null; - const recordIds = message.match(/\brec[A-Za-z0-9]{10,}/g) ?? []; - return { tableId, fieldId, recordIds }; -}; - const shouldForceFlush = (batcher: BatcherState, flushIntervalMs: number) => { // Hard upper bound: flush at most 5× the interval after the first message // This prevents never flushing if messages keep coming in at a high rate, while still allowing for bursts of messages to be batched together @@ -225,17 +223,15 @@ const flushBatcher = async (batcher: BatcherState) => { // Build main message if (batch.occurrences > 1) { - const tableInfo = batch.tableId ? ` (Table: ${batch.tableId})` : ''; - const fieldInfo = batch.fieldId ? ` (Field: ${batch.fieldId})` : ''; - const header = `${mainMessage}${tableInfo}${fieldInfo}\n\n⚠️ This error occurred ${batch.occurrences} times`; + const header = `${mainMessage}\n\n⚠️ This error occurred ${batch.occurrences} times`; - // Only Airtable-derived warnings carry record IDs; other batched errors - // (e.g. tRPC server errors) have none, so drop the record clause for them. - if (batch.affectedRecords.size > 0) { - const recordList = Array.from(batch.affectedRecords).slice(0, 10).join(', '); - const moreRecords = batch.affectedRecords.size > 10 ? ` and ${batch.affectedRecords.size - 10} more` : ''; + // Only callers that pass dedupeKeys carry affected items; others (e.g. tRPC + // server errors) have none, so drop the item clause for them. + if (batch.dedupedItems.size > 0) { + const itemList = Array.from(batch.dedupedItems).slice(0, 10).join(', '); + const moreItems = batch.dedupedItems.size > 10 ? ` and ${batch.dedupedItems.size - 10} more` : ''; - messages.push(`${header} affecting ${batch.affectedRecords.size} record(s):\n${recordList}${moreRecords}`); + messages.push(`${header} affecting ${batch.dedupedItems.size} item(s):\n${itemList}${moreItems}`); } else { messages.push(`${header}.`); } @@ -271,9 +267,9 @@ const maybeEscalateSpike = async (batcher: BatcherState, batch: MessageBatch, ma return; } - // Distinct records is the blast radius; fall back to raw occurrences when a - // warning carries no record IDs (e.g. a retry loop on the same broken field). - const spikeCount = batch.affectedRecords.size > 0 ? batch.affectedRecords.size : batch.occurrences; + // Distinct affected items is the blast radius; fall back to raw occurrences when a + // batch carries no dedupeKeys (e.g. a retry loop on the same broken field). + const spikeCount = batch.dedupedItems.size > 0 ? batch.dedupedItems.size : batch.occurrences; if (spikeCount < spikeThreshold) { return; }