Skip to content

Commit

Permalink
perf(NODE-6452): Optimize CommandStartedEvent and CommandSucceededEve…
Browse files Browse the repository at this point in the history 
…nt constructors (#4371)
  • Loading branch information
@W-A-James
W-A-James authored Jan 17, 2025
1 parent cba0a2a commit 41b066b
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 137 deletions.
65 changes: 10 additions & 55 deletions src/cmap/command_monitoring_events.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) ||
Expand Down Expand Up @@ -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) {
Expand All @@ -276,72 +266,37 @@ 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] };
}
});
}

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<string, unknown> = {};
const clonedCommand: Record<string, unknown> = {};
let clonedQuery: Record<string, unknown> = {};
const clonedCommand: Record<string, unknown> = { ...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<string, unknown>)[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) {
Expand Down
37 changes: 0 additions & 37 deletions src/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>(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<T> = {
value: T;
next: ListNode<T> | HeadNode<T>;
Expand Down
45 changes: 0 additions & 45 deletions test/integration/command-logging-and-monitoring/command_monitoring.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
});
});

0 comments on commit 41b066b

Please sign in to comment.