-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(NODE-4686): Add log messages to CLAM #3955
Changes from 18 commits
6d32c32
5f5d77e
209ba27
3bee17a
fcfc98b
422c6a9
c360ec2
054094f
0d8d051
383f890
7033dc8
1d70579
0ae8792
e92849d
9776ec9
3332f94
cacaed2
fa223cf
299e8a1
1181991
8f6c860
dc93151
cbb5df6
9ace7a0
e9d4f8d
7892601
3c18428
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 |
---|---|---|
@@ -1,12 +1,8 @@ | ||
import { type Document, EJSON, type EJSONOptions } from 'bson'; | ||
import { type Document, EJSON, type EJSONOptions, type ObjectId } from 'bson'; | ||
nbbeeken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import type { Writable } from 'stream'; | ||
import { inspect } from 'util'; | ||
|
||
import type { | ||
CommandFailedEvent, | ||
CommandStartedEvent, | ||
CommandSucceededEvent | ||
} from './cmap/command_monitoring_events'; | ||
import type { CommandStartedEvent } from './cmap/command_monitoring_events'; | ||
import type { | ||
ConnectionCheckedInEvent, | ||
ConnectionCheckedOutEvent, | ||
|
@@ -295,6 +291,40 @@ function compareSeverity(s0: SeverityLevel, s1: SeverityLevel): 1 | 0 | -1 { | |
return s0Num < s1Num ? -1 : s0Num > s1Num ? 1 : 0; | ||
} | ||
|
||
durran marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* @internal | ||
* Must be separate from Events API due to differences in spec requirements for logging a command success | ||
*/ | ||
export type LoggableCommandSucceededEvent = { | ||
address: string; | ||
connectionId?: string | number; | ||
requestId: number; | ||
duration: number; | ||
commandName: string; | ||
reply: Document | undefined; | ||
serviceId?: ObjectId; | ||
name: typeof COMMAND_SUCCEEDED; | ||
serverConnectionId: bigint | null; | ||
databaseName: string; | ||
}; | ||
|
||
/** | ||
* @internal | ||
* Must be separate from Events API due to differences in spec requirements for logging a command failure | ||
*/ | ||
export type LoggableCommandFailedEvent = { | ||
address: string; | ||
connectionId?: string | number; | ||
requestId: number; | ||
duration: number; | ||
commandName: string; | ||
failure: Error; | ||
serviceId?: ObjectId; | ||
name: typeof COMMAND_FAILED; | ||
serverConnectionId: bigint | null; | ||
databaseName: string; | ||
}; | ||
|
||
/** | ||
* @internal | ||
* Must be separate from Events API due to differences in spec requirements for logging server heartbeat beginning | ||
|
@@ -350,8 +380,8 @@ export type LoggableEvent = | |
| ServerSelectionSucceededEvent | ||
| WaitingForSuitableServerEvent | ||
| CommandStartedEvent | ||
| CommandSucceededEvent | ||
| CommandFailedEvent | ||
| LoggableCommandSucceededEvent | ||
| LoggableCommandFailedEvent | ||
| ConnectionPoolCreatedEvent | ||
| ConnectionPoolReadyEvent | ||
| ConnectionPoolClosedEvent | ||
|
@@ -383,7 +413,8 @@ export function stringifyWithMaxLen( | |
maxDocumentLength: number, | ||
options: EJSONOptions = {} | ||
): string { | ||
let strToTruncate: string; | ||
let strToTruncate = ''; | ||
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. Non-blocking and definitely only tangentially related: the 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 something we will solve / take a further look at in NODE-5839. |
||
|
||
if (typeof value === 'function') { | ||
strToTruncate = value.toString(); | ||
} else { | ||
|
@@ -420,7 +451,7 @@ function attachServerSelectionFields( | |
|
||
function attachCommandFields( | ||
log: Record<string, any>, | ||
commandEvent: CommandStartedEvent | CommandSucceededEvent | CommandFailedEvent | ||
commandEvent: CommandStartedEvent | LoggableCommandSucceededEvent | LoggableCommandFailedEvent | ||
) { | ||
log.commandName = commandEvent.commandName; | ||
log.requestId = commandEvent.requestId; | ||
|
@@ -431,6 +462,8 @@ function attachCommandFields( | |
if (commandEvent?.serviceId) { | ||
log.serviceId = commandEvent.serviceId.toHexString(); | ||
} | ||
log.databaseName = commandEvent.databaseName; | ||
log.serverConnectionId = commandEvent?.serverConnectionId; | ||
|
||
return log; | ||
} | ||
|
@@ -490,20 +523,20 @@ function defaultLogTransform( | |
case COMMAND_STARTED: | ||
log = attachCommandFields(log, logObject); | ||
log.message = 'Command started'; | ||
log.command = stringifyWithMaxLen(logObject.command, maxDocumentLength); | ||
log.command = stringifyWithMaxLen(logObject.command, maxDocumentLength, { relaxed: true }); | ||
log.databaseName = logObject.databaseName; | ||
return log; | ||
case COMMAND_SUCCEEDED: | ||
log = attachCommandFields(log, logObject); | ||
log.message = 'Command succeeded'; | ||
log.durationMS = logObject.duration; | ||
log.reply = stringifyWithMaxLen(logObject.reply, maxDocumentLength); | ||
log.reply = stringifyWithMaxLen(logObject.reply, maxDocumentLength, { relaxed: true }); | ||
nbbeeken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return log; | ||
case COMMAND_FAILED: | ||
log = attachCommandFields(log, logObject); | ||
log.message = 'Command failed'; | ||
log.durationMS = logObject.duration; | ||
log.failure = logObject.failure; | ||
log.failure = logObject.failure.message ?? '{}'; | ||
nbbeeken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return log; | ||
case CONNECTION_POOL_CREATED: | ||
log = attachConnectionFields(log, logObject); | ||
|
@@ -701,12 +734,16 @@ export class MongoLogger { | |
this.logDestination = options.logDestination; | ||
} | ||
|
||
willLog(severity: SeverityLevel, component: MongoLoggableComponent): boolean { | ||
return compareSeverity(severity, this.componentSeverities[component]) <= 0; | ||
} | ||
|
||
private log( | ||
severity: SeverityLevel, | ||
component: MongoLoggableComponent, | ||
message: Loggable | string | ||
): void { | ||
if (compareSeverity(severity, this.componentSeverities[component]) > 0) return; | ||
if (!this.willLog(severity, component)) return; | ||
|
||
let logMessage: Log = { t: new Date(), c: component, s: severity }; | ||
if (typeof message === 'string') { | ||
|
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 is an interesting idea - I see it's used in TypedEventEmitter to know which logging component to use.
Will there ever be a case where
emitAndLogCommand
will be used to log for a component that is notMongoLoggableComponent.COMMAND
?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.
No, there won't. The extra
this.component
check should ensure that anemitAndLog
or variant function (emitAndLogCommand, emitAndLogHeartbeat
) is not called from any class for which the logging component is not defined already.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.
I can add in an extra check such as:
this.component === MongoLoggableComponent.COMMAND
, remove thethis.component
check foremitAndLogCommand
, or keep it the same. What do you think?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.
It's strange to me that we have a dedicated method for logging commands, but to use it we also need to set the component on the class. I would expect that either:
emitAndLogCommand()
, and not thecomponent
property.emitAndLog()
method that uses thecomponent
property and no specialized methods on the TypedEventEmitterIf we want to prevent the emitAndLog functions from being called, maybe we could use inheritance for this? that way we could only define the log methods where they can be used
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.
That's true. For the sake of this PR, I'll just hardcode in the component in
emitAndLogCommand
asMongoLoggableComponent.COMMAND.
Since I realizeemitAndLogCommand
already implies that the command component is being utilized.