-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add description option and fix resolution noise #11
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
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,10 @@ | ||
| --- | ||
| "@iqai/alert-logger": minor | ||
| --- | ||
|
|
||
| feat: add `description` option and fix resolution noise | ||
|
|
||
| - Add `description` field to `AlertOptions` for separating short titles from detailed messages. When set, `description` is used as the embed body instead of the title. | ||
| - Allow `error()` and `critical()` to accept `(title, options)` without an intermediate `undefined` error param. | ||
| - Resolution notifications now only fire for sustained incidents (count > rampThreshold). One-off or sporadic alerts no longer produce "Resolved" messages. | ||
| - NestJS exception filter uses `{METHOD} {PATH}` as the alert title instead of the full error message. |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -76,22 +76,24 @@ export class AlertLogger { | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| info(title: string, options?: AlertOptions): void { | ||||||||||||||
| this.log('info', title, title, undefined, options) | ||||||||||||||
| const message = options?.description ?? title | ||||||||||||||
| this.log('info', title, message, undefined, options) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| warn(title: string, options?: AlertOptions): void { | ||||||||||||||
| this.log('warning', title, title, undefined, options) | ||||||||||||||
| const message = options?.description ?? title | ||||||||||||||
| this.log('warning', title, message, undefined, options) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| error(title: string, error?: Error | string, options?: AlertOptions): void { | ||||||||||||||
| error(title: string, error?: Error | string | AlertOptions, options?: AlertOptions): void { | ||||||||||||||
| const [err, opts] = this.normalizeErrorArgs(error, options) | ||||||||||||||
| const message = err?.message ?? title | ||||||||||||||
| const message = opts?.description ?? err?.message ?? title | ||||||||||||||
| this.log('critical', title, message, err, opts) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| critical(title: string, error?: Error | string, options?: AlertOptions): void { | ||||||||||||||
| critical(title: string, error?: Error | string | AlertOptions, options?: AlertOptions): void { | ||||||||||||||
| const [err, opts] = this.normalizeErrorArgs(error, options) | ||||||||||||||
| const message = err?.message ?? title | ||||||||||||||
| const message = opts?.description ?? err?.message ?? title | ||||||||||||||
| this.log('critical', title, message, err, opts) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -227,9 +229,13 @@ export class AlertLogger { | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private normalizeErrorArgs( | ||||||||||||||
| error?: Error | string, | ||||||||||||||
| error?: Error | string | AlertOptions, | ||||||||||||||
| options?: AlertOptions, | ||||||||||||||
| ): [Error | undefined, AlertOptions | undefined] { | ||||||||||||||
| // Allow error("title", { description: ... }) without an intermediate undefined | ||||||||||||||
| if (error != null && typeof error === 'object' && !(error instanceof Error)) { | ||||||||||||||
| return [undefined, error] | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+236
to
+238
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. The current implementation of the overload ignores the
Suggested change
|
||||||||||||||
| if (typeof error === 'string') { | ||||||||||||||
| return [new Error(error), options] | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,12 +16,21 @@ export class AlertLoggerService { | |||||||||||||||||||||
| this.logger.warn(title, this.mergeContext(options)) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| error(title: string, error?: Error | string, options?: AlertOptions): void { | ||||||||||||||||||||||
| this.logger.error(title, error, this.mergeContext(options)) | ||||||||||||||||||||||
| error(title: string, error?: Error | string | AlertOptions, options?: AlertOptions): void { | ||||||||||||||||||||||
| // When called as error("title", { ... }), pass through directly | ||||||||||||||||||||||
| 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)) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+21
to
+25
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 overload implementation ignores the
Suggested change
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| critical(title: string, error?: Error | string, options?: AlertOptions): void { | ||||||||||||||||||||||
| this.logger.critical(title, error, this.mergeContext(options)) | ||||||||||||||||||||||
| critical(title: string, error?: Error | string | AlertOptions, options?: AlertOptions): void { | ||||||||||||||||||||||
| 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)) | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+29
to
+33
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. Similar to the
Suggested change
|
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| private mergeContext(options?: AlertOptions): AlertOptions { | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,18 +24,20 @@ export class AlertExceptionFilter extends BaseExceptionFilter { | |
| const path = request?.url ?? 'UNKNOWN' | ||
| const ip = request?.ip ?? 'UNKNOWN' | ||
|
|
||
| const title = `${method} ${path}` | ||
|
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. Using the raw request path in the alert title can significantly degrade deduplication quality if the path contains dynamic parameters (e.g., |
||
|
|
||
| if (exception instanceof HttpException) { | ||
| const statusCode = exception.getStatus() | ||
|
|
||
| if (statusCode >= 500) { | ||
| this.alert.error(exception.message, exception, { | ||
| this.alert.error(title, exception, { | ||
| fields: { method, path, statusCode, ip }, | ||
| }) | ||
| } | ||
| } else { | ||
| const error = exception instanceof Error ? exception : new Error(String(exception)) | ||
|
|
||
| this.alert.critical(error.message, error, { | ||
| this.alert.critical(title, error, { | ||
| fields: { method, path, statusCode: 500, ip }, | ||
| }) | ||
| } | ||
|
|
||
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.
This conditional logic introduces a memory leak in
AlertLogger. TheAlertLoggermaintains analertMetamap that is only cleaned up whenhandleResolutionis 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,
checkResolutionsshould 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 asustainedflag to theResolvedEntryinterface).