Skip to content

Commit

Permalink
fix(mongodb): resolve collection name for getMore command (#3919)
Browse files Browse the repository at this point in the history
  • Loading branch information
@david-luna
david-luna authored Mar 19, 2024
1 parent 41c8dcf commit 990442a
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 27 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ See the <<upgrade-to-v4>> guide.
[float]
===== Bug fixes
* Fix span names for `getMore` command of mongodb. ({pull}3919[#3919])
* Fix instrumentation of mongodb to not break [email protected]. Mongodb v6.4.0
included changes that resulted in the APM agent's instrumentation breaking it.
({pull}3897[#3897])
Expand Down
31 changes: 31 additions & 0 deletions lib/instrumentation/modules/mongodb.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,37 @@ module.exports = (mongodb, agent, { version, enabled }) => {
}

function collectionFor(event) {
// `getMore` command is a special case where the collection name is
// placed in a property named `collection`. The types exported
// do not ensure the property is there so we are defensive on its
// retrieval.
//
// Example of `getMore` payload:
// {
// event: CommandStartedEvent {
// name: 'commandStarted',
// address: '172.20.0.2:27017',
// connectionId: 12,
// requestId: 686,
// databaseName: 'mydatabase',
// commandName: 'getMore',
// command: {
// getMore: new Long('1769182660590360229'),
// collection: 'Interaction',
// ...
// }
// },
// commandName: 'getMore',
// collection: new Long('1769182660590360229')
// }
// ref: https://github.com/elastic/apm-agent-nodejs/issues/3834
if (
event.commandName === 'getMore' &&
typeof event.command.collection === 'string'
) {
return event.command.collection;
}

const collection = event.command[event.commandName];
return typeof collection === 'string' ? collection : '$cmd';
}
Expand Down
85 changes: 66 additions & 19 deletions test/instrumentation/modules/mongodb/fixtures/use-mongodb-async-context.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ const http = require('http');
const MongoClient = require('mongodb').MongoClient;

// ---- support functions
async function makeRequest(url) {
return new Promise((resolve, reject) => {
http.request(url).on('response', resolve).on('error', reject).end();
});
}

/**
*
* @param {import('mongodb').MongoClient} mongodbClient
Expand All @@ -29,15 +35,18 @@ async function useMongodbAsyncContext(options) {
const { port } = options;
const serverUrl = `http://localhost:${port}`;

const reqs = new Array(50).fill(serverUrl).map((url) => {
return new Promise((resolve, reject) => {
http.request(url).on('response', resolve).on('error', reject).end();
});
});
// 1st fill with some data
await makeRequest(`${serverUrl}/insert`);

// Wait for all request to finish and make sure APM Server
// receives all spans
const reqs = new Array(50).fill(`${serverUrl}/find`).map(makeRequest);

// Wait for all request to finish
await Promise.all(reqs);

// Clear the data
await makeRequest(`${serverUrl}/delete`);

// Make sure APM Server receives all spans
await apm.flush();
}

Expand All @@ -54,20 +63,58 @@ async function main() {
const server = http.createServer(function (req, res) {
req.resume();
req.on('end', function () {
mongodbClient
.db(db)
.collection(col)
.find()
.toArray()
.then(JSON.stringify)
.then(function (body) {
res.writeHead(200, {
server: 'trace-mongodb-cats-server',
'content-type': 'text/plain',
'content-length': Buffer.byteLength(body),
const successBody = JSON.stringify({ success: true });
const failureBody = JSON.stringify({ success: true });
if (req.url === '/insert') {
const items = new Array(500).fill({}).map((_, num) => ({ num }));
mongodbClient
.db(db)
.collection(col)
.insertMany(items)
.then(function () {
res.writeHead(200, {
server: 'trace-mongodb-cats-server',
'content-type': 'text/plain',
'content-length': Buffer.byteLength(successBody),
});
res.end(successBody);
});
} else if (req.url === '/find') {
mongodbClient
.db(db)
.collection(col)
.find()
.toArray()
.then(JSON.stringify)
.then(function (body) {
res.writeHead(200, {
server: 'trace-mongodb-cats-server',
'content-type': 'text/plain',
'content-length': Buffer.byteLength(body),
});
res.end(body);
});
} else if (req.url === '/delete') {
mongodbClient
.db(db)
.collection(col)
.deleteMany({})
.then(function (body) {
res.writeHead(200, {
server: 'trace-mongodb-cats-server',
'content-type': 'text/plain',
'content-length': Buffer.byteLength(successBody),
});
res.end(successBody);
});
res.end(body);
} else {
res.writeHead(200, {
server: 'trace-mongodb-cats-server',
'content-type': 'text/plain',
'content-length': Buffer.byteLength(failureBody),
});
res.end(failureBody);
}
});
});
server.listen();
Expand Down
60 changes: 52 additions & 8 deletions test/instrumentation/modules/mongodb/mongodb.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const testFixtures = [
TEST_COLLECTION,
TEST_USE_CALLBACKS: String(TEST_USE_CALLBACKS),
},
verbose: true,
verbose: false,
checkApmServer: (t, apmServer) => {
t.ok(apmServer.events[0].metadata, 'metadata');
const events = sortApmEvents(apmServer.events);
Expand Down Expand Up @@ -414,7 +414,7 @@ const testFixtures = [
TEST_COLLECTION,
TEST_USE_CALLBACKS: String(TEST_USE_CALLBACKS),
},
verbose: true,
verbose: false,
checkApmServer: (t, apmServer) => {
t.ok(apmServer.events[0].metadata, 'metadata');
const events = sortApmEvents(apmServer.events);
Expand Down Expand Up @@ -501,6 +501,15 @@ const testFixtures = [
TEST_DB,
TEST_COLLECTION,
},
// The `getMore` command seems to be queued outside the connection pool
// for versions <4.11.0 and as a result the `find` command is properly
// linked to the parent transaction but not the `getMore` commands from
// the cursor. Since v4.11.0 was published in 2022-09-19 there was a decision
// to skip this test for earlier version
// Ref: https://github.com/elastic/apm-agent-nodejs/pull/3919#issuecomment-2005283132
versionRanges: {
mongodb: '>=4.11.0',
},
verbose: false,
checkApmServer: (t, apmServer) => {
t.ok(apmServer.events[0].metadata, 'metadata');
Expand All @@ -514,17 +523,52 @@ const testFixtures = [
.filter((e) => e.span && e.span.type !== 'external')
.map((e) => e.span);

while (transactions.length) {
const tx = transactions.shift();
const idx = spans.findIndex((s) => s.parent_id === tx.id);
const extractSpans = (tx) => {
const result = [];
let i = 0;

while (i < spans.length) {
if (spans[i].parent_id === tx.id) {
result.push(...spans.splice(i, 1));
} else {
i++;
}
}

return result;
};

let tx = transactions.shift();
let txSpans = extractSpans(tx);

// Assertions for insert transaction
t.ok(tx, 'insert transaction');
t.ok(txSpans.length === 1, 'insert spans length');
t.equal(txSpans[0].name, 'elasticapm.test.insert', 'span.name');

t.ok(idx !== -1, 'transaction has a child span');
// Assertions for all find transactions
while (transactions.length - 1) {
tx = transactions.shift();
txSpans = extractSpans(tx);

const [span] = spans.splice(idx, 1);
t.ok(txSpans.length > 0, 'transaction has child spans');

t.equal(span.name, 'elasticapm.test.find', 'span.name');
txSpans.forEach((s, idx) => {
if (idx === 0) {
t.equal(s.name, 'elasticapm.test.find', 'span.name');
} else {
t.equal(s.name, 'elasticapm.test.getMore', 'span.name');
}
});
}

// Assertions for delete transaction
tx = transactions.shift();
txSpans = extractSpans(tx);

t.ok(txSpans.length === 1, 'delete spans length');
t.equal(txSpans[0].name, 'elasticapm.test.delete', 'span.name');

t.equal(spans.length, 0, 'all spans accounted for');
},
},
Expand Down

0 comments on commit 990442a

Please sign in to comment.