Skip to content
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

fix(mongodb): resolve collections for command #3919

Merged
merged 7 commits into from
Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
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 mongodb@6.4.0. 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
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
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
47 changes: 41 additions & 6 deletions test/instrumentation/modules/mongodb/mongodb.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -514,17 +514,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
Loading