feat: add description option and fix resolution noise#11
Conversation
- Add `description` field to AlertOptions for separating short titles
from detailed messages in Discord embeds
- Allow error()/critical() to accept (title, options) directly without
an intermediate undefined error parameter
- Resolution notifications now only fire for sustained incidents
(count > rampThreshold, default 64) — sporadic failures no longer
produce noisy "Resolved" messages
- NestJS exception filter uses {METHOD} {PATH} as the alert title
instead of the full error.message
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a description field for detailed alert messages, simplifies method signatures for direct option passing, and restricts resolution notifications to sustained incidents. Feedback highlights a potential memory leak in the aggregator due to incomplete cleanup of low-count alerts and identifies data loss in the new method overloads where the third argument is ignored. Additionally, the reviewer suggests using more stable titles in the NestJS exception filter to avoid degrading deduplication quality with dynamic URL paths.
| if (state.count > this.config.rampThreshold) { | ||
| resolved.push({ | ||
| fingerprint, | ||
| count: state.count, | ||
| firstSeen: state.firstSeen, | ||
| lastSeen: state.lastSeen, | ||
| peakRate: state.peakRate, | ||
| }) | ||
| } |
There was a problem hiding this comment.
This conditional logic introduces a memory leak in AlertLogger. The AlertLogger maintains an alertMeta map that is only cleaned up when handleResolution is called. By filtering out low-count resolutions here, the corresponding metadata for sporadic or one-off alerts will never be removed from memory, causing the map to grow indefinitely over time.
To fix this, checkResolutions should return all resolved fingerprints so the logger can prune its metadata, while the logger itself should decide whether to actually send a notification based on the count (e.g., by adding a sustained flag to the ResolvedEntry interface).
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | ||
| return [undefined, error] | ||
| } |
There was a problem hiding this comment.
The current implementation of the overload ignores the options argument if the error argument is an object. This means calls like logger.error("title", { description: "..." }, { tags: ["foo"] }) will lose the tags and other options provided in the third argument.
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | |
| return [undefined, error] | |
| } | |
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | |
| return [undefined, { ...error, ...options }] | |
| } |
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | ||
| this.logger.error(title, this.mergeContext(error)) | ||
| } else { | ||
| this.logger.error(title, error, this.mergeContext(options)) | ||
| } |
There was a problem hiding this comment.
This overload implementation ignores the options argument when error is an object. Consider merging the options to ensure no metadata (like tags or custom fields) is lost when using the two-argument style alongside global context.
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | |
| this.logger.error(title, this.mergeContext(error)) | |
| } else { | |
| this.logger.error(title, error, this.mergeContext(options)) | |
| } | |
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | |
| this.logger.error(title, this.mergeContext({ ...error, ...options })) | |
| } else { | |
| this.logger.error(title, error, this.mergeContext(options)) | |
| } |
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | ||
| this.logger.critical(title, this.mergeContext(error)) | ||
| } else { | ||
| this.logger.critical(title, error, this.mergeContext(options)) | ||
| } |
There was a problem hiding this comment.
Similar to the error method, this implementation ignores the options argument when error is an object. Merging them ensures consistency and prevents data loss.
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | |
| this.logger.critical(title, this.mergeContext(error)) | |
| } else { | |
| this.logger.critical(title, error, this.mergeContext(options)) | |
| } | |
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | |
| this.logger.critical(title, this.mergeContext({ ...error, ...options })) | |
| } else { | |
| this.logger.critical(title, error, this.mergeContext(options)) | |
| } |
| const path = request?.url ?? 'UNKNOWN' | ||
| const ip = request?.ip ?? 'UNKNOWN' | ||
|
|
||
| const title = `${method} ${path}` |
There was a problem hiding this comment.
Using the raw request path in the alert title can significantly degrade deduplication quality if the path contains dynamic parameters (e.g., /users/123). Since the path is already included in the fields metadata (line 34), consider using a more stable title (like the exception name or a generic route pattern) to ensure similar errors are grouped correctly by the aggregator.
1 participant
Summary
descriptionfield toAlertOptions— lets callers pass a short title + detailed body separately, so Discord embeds have clean titles like[CRITICAL] Market Resolution Failedinstead of stuffing the full error message into the titleerror()/critical()to accept(title, options)directly without an awkwardundefinederror param in the middlerampThreshold, default 64). One-off or sporadic failures no longer produce noisy "Resolved" messages{METHOD} {PATH}as the alert title instead oferror.messageTest plan
descriptionoption (4 tests)error(title, options)works withoutundefined🤖 Generated with Claude Code
Closes #15