From 41b066b2b286879085b05a58c0486aa1d24303bc Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 17 Jan 2025 18:16:50 -0500 Subject: [PATCH] perf(NODE-6452): Optimize CommandStartedEvent and CommandSucceededEvent constructors (#4371) --- src/cmap/command_monitoring_events.ts | 65 +++---------------- src/utils.ts | 37 ----------- .../command_monitoring.test.ts | 45 ------------- 3 files changed, 10 insertions(+), 137 deletions(-) diff --git a/src/cmap/command_monitoring_events.ts b/src/cmap/command_monitoring_events.ts index a7e1653becc..992b9dc47c9 100644 --- a/src/cmap/command_monitoring_events.ts +++ b/src/cmap/command_monitoring_events.ts @@ -6,7 +6,7 @@ import { LEGACY_HELLO_COMMAND, LEGACY_HELLO_COMMAND_CAMEL_CASE } from '../constants'; -import { calculateDurationInMs, deepCopy } from '../utils'; +import { calculateDurationInMs } from '../utils'; import { DocumentSequence, OpMsgRequest, @@ -125,7 +125,7 @@ export class CommandSucceededEvent { this.requestId = command.requestId; this.commandName = commandName; this.duration = calculateDurationInMs(started); - this.reply = maybeRedact(commandName, cmd, extractReply(command, reply)); + this.reply = maybeRedact(commandName, cmd, extractReply(reply)); this.serverConnectionId = serverConnectionId; } @@ -214,7 +214,6 @@ const HELLO_COMMANDS = new Set(['hello', LEGACY_HELLO_COMMAND, LEGACY_HELLO_COMM // helper methods const extractCommandName = (commandDoc: Document) => Object.keys(commandDoc)[0]; -const namespace = (command: OpQueryRequest) => command.ns; const collectionName = (command: OpQueryRequest) => command.ns.split('.')[1]; const maybeRedact = (commandName: string, commandDoc: Document, result: Error | Document) => SENSITIVE_COMMANDS.has(commandName) || @@ -242,19 +241,10 @@ const LEGACY_FIND_OPTIONS_MAP = { returnFieldSelector: 'projection' } as const; -const OP_QUERY_KEYS = [ - 'tailable', - 'oplogReplay', - 'noCursorTimeout', - 'awaitData', - 'partial', - 'exhaust' -] as const; - /** Extract the actual command from the query, possibly up-converting if it's a legacy format */ function extractCommand(command: WriteProtocolMessageType): Document { if (command instanceof OpMsgRequest) { - const cmd = deepCopy(command.command); + const cmd = { ...command.command }; // For OP_MSG with payload type 1 we need to pull the documents // array out of the document sequence for monitoring. if (cmd.ops instanceof DocumentSequence) { @@ -276,7 +266,7 @@ function extractCommand(command: WriteProtocolMessageType): Document { result = { find: collectionName(command) }; Object.keys(LEGACY_FIND_QUERY_MAP).forEach(key => { if (command.query[key] != null) { - result[LEGACY_FIND_QUERY_MAP[key]] = deepCopy(command.query[key]); + result[LEGACY_FIND_QUERY_MAP[key]] = { ...command.query[key] }; } }); } @@ -284,64 +274,29 @@ function extractCommand(command: WriteProtocolMessageType): Document { Object.keys(LEGACY_FIND_OPTIONS_MAP).forEach(key => { const legacyKey = key as keyof typeof LEGACY_FIND_OPTIONS_MAP; if (command[legacyKey] != null) { - result[LEGACY_FIND_OPTIONS_MAP[legacyKey]] = deepCopy(command[legacyKey]); - } - }); - - OP_QUERY_KEYS.forEach(key => { - if (command[key]) { - result[key] = command[key]; + result[LEGACY_FIND_OPTIONS_MAP[legacyKey]] = command[legacyKey]; } }); - if (command.pre32Limit != null) { - result.limit = command.pre32Limit; - } - - if (command.query.$explain) { - return { explain: result }; - } return result; } - const clonedQuery: Record = {}; - const clonedCommand: Record = {}; + let clonedQuery: Record = {}; + const clonedCommand: Record = { ...command }; if (command.query) { - for (const k in command.query) { - clonedQuery[k] = deepCopy(command.query[k]); - } + clonedQuery = { ...command.query }; clonedCommand.query = clonedQuery; } - for (const k in command) { - if (k === 'query') continue; - clonedCommand[k] = deepCopy((command as unknown as Record)[k]); - } return command.query ? clonedQuery : clonedCommand; } -function extractReply(command: WriteProtocolMessageType, reply?: Document) { +function extractReply(reply?: Document) { if (!reply) { return reply; } - if (command instanceof OpMsgRequest) { - return deepCopy(reply.result ? reply.result : reply); - } - - // is this a legacy find command? - if (command.query && command.query.$query != null) { - return { - ok: 1, - cursor: { - id: deepCopy(reply.cursorId), - ns: namespace(command), - firstBatch: deepCopy(reply.documents) - } - }; - } - - return deepCopy(reply.result ? reply.result : reply); + return reply.result ? reply.result : reply; } function extractConnectionDetails(connection: Connection) { diff --git a/src/utils.ts b/src/utils.ts index c23161612a8..55cf455676b 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -620,43 +620,6 @@ export function isRecord( return true; } -/** - * Make a deep copy of an object - * - * NOTE: This is not meant to be the perfect implementation of a deep copy, - * but instead something that is good enough for the purposes of - * command monitoring. - */ -export function deepCopy(value: T): T { - if (value == null) { - return value; - } else if (Array.isArray(value)) { - return value.map(item => deepCopy(item)) as unknown as T; - } else if (isRecord(value)) { - const res = {} as any; - for (const key in value) { - res[key] = deepCopy(value[key]); - } - return res; - } - - const ctor = (value as any).constructor; - if (ctor) { - switch (ctor.name.toLowerCase()) { - case 'date': - return new ctor(Number(value)); - case 'map': - return new Map(value as any) as unknown as T; - case 'set': - return new Set(value as any) as unknown as T; - case 'buffer': - return Buffer.from(value as unknown as Buffer) as unknown as T; - } - } - - return value; -} - type ListNode = { value: T; next: ListNode | HeadNode; diff --git a/test/integration/command-logging-and-monitoring/command_monitoring.test.ts b/test/integration/command-logging-and-monitoring/command_monitoring.test.ts index 124d5e7f9a2..eb589346a0c 100644 --- a/test/integration/command-logging-and-monitoring/command_monitoring.test.ts +++ b/test/integration/command-logging-and-monitoring/command_monitoring.test.ts @@ -603,49 +603,4 @@ describe('Command Monitoring', function () { }); } }); - - describe('Internal state references', function () { - let client; - - beforeEach(function () { - client = this.configuration.newClient( - { writeConcern: { w: 1 } }, - { maxPoolSize: 1, monitorCommands: true } - ); - }); - - afterEach(function (done) { - client.close(done); - }); - - // NODE-1502 - it('should not allow mutation of internal state from commands returned by event monitoring', function () { - const started = []; - const succeeded = []; - client.on('commandStarted', filterForCommands('insert', started)); - client.on('commandSucceeded', filterForCommands('insert', succeeded)); - const documentToInsert = { a: { b: 1 } }; - const db = client.db(this.configuration.db); - return db - .collection('apm_test') - .insertOne(documentToInsert) - .then(r => { - expect(r).to.have.property('insertedId').that.is.an('object'); - expect(started).to.have.lengthOf(1); - // Check if contents of returned document are equal to document inserted (by value) - expect(documentToInsert).to.deep.equal(started[0].command.documents[0]); - // Check if the returned document is a clone of the original. This confirms that the - // reference is not the same. - expect(documentToInsert !== started[0].command.documents[0]).to.equal(true); - expect(documentToInsert.a !== started[0].command.documents[0].a).to.equal(true); - - started[0].command.documents[0].a.b = 2; - expect(documentToInsert.a.b).to.equal(1); - - expect(started[0].commandName).to.equal('insert'); - expect(started[0].command.insert).to.equal('apm_test'); - expect(succeeded).to.have.lengthOf(1); - }); - }); - }); });