From 808323f0832952870fd1e94474b3fd3e0ab1b8c4 Mon Sep 17 00:00:00 2001 From: Bob Evans Date: Thu, 4 Apr 2024 14:43:04 -0400 Subject: [PATCH] refactor(mongodb): Removed instrumentation that handles connecting via unix domain socket. (#2129) --- lib/instrumentation/mongodb/common.js | 50 ++---------- lib/instrumentation/mongodb/v3-mongo.js | 15 ++-- lib/instrumentation/mongodb/v4-mongo.js | 28 +++---- test/versioned/mongodb/collection-common.js | 89 --------------------- test/versioned/mongodb/common.js | 13 --- test/versioned/mongodb/db-common.js | 39 --------- test/versioned/mongodb/legacy/cursor.tap.js | 2 +- 7 files changed, 23 insertions(+), 213 deletions(-) diff --git a/lib/instrumentation/mongodb/common.js b/lib/instrumentation/mongodb/common.js index 94878af119..3cd7582c57 100644 --- a/lib/instrumentation/mongodb/common.js +++ b/lib/instrumentation/mongodb/common.js @@ -11,7 +11,6 @@ const { } = require('../../shim/specs') const { CURSOR_OPS, COLLECTION_OPS, DB_OPS } = require('./constants') const common = module.exports -common.NR_ATTRS = Symbol('NR_ATTRS') /** * Instruments all methods from constants.CURSOR_OPS on a given @@ -88,8 +87,7 @@ common.instrumentDb = function instrumentDb(shim, Db) { * Sets up the desc for all instrumented query methods * * @param {Shim} shim instance of shim - * @param {Function} fn function getting instrumented - * @param _fn + * @param {Function} _fn function getting instrumented * @param {string} methodName name of function * @returns {QuerySpec} query spec */ @@ -156,13 +154,10 @@ common.captureAttributesOnStarted = function captureAttributesOnStarted( if (connId) { // used in v3 when connection is a cluster pool if (typeof connId === 'number') { - setHostPort(shim, evnt.address, evnt.databaseName, this.$MongoClient) + setHostPort(shim, evnt.address, evnt.databaseName) // used in v3 when connection is to 1 host } else if (typeof connId === 'string') { setHostPort(shim, connId, evnt.databaseName) - // v2 contains `domainSocket`, get socket connection from `host` - } else if (connId.domainSocket) { - shim.captureInstanceAttributes('localhost', connId.host, evnt.databaseName) // v2 remote connection get `host` `port` from respective properties } else { shim.captureInstanceAttributes(connId.host, connId.port, evnt.databaseName) @@ -173,37 +168,14 @@ common.captureAttributesOnStarted = function captureAttributesOnStarted( /** * Extracts the host and port from a connection string - * This also handles if connection string is a domain socket - * Mongo sticks the path to the domain socket in the "host" slot, but we - * want it in the "port", so if we have a domain socket we need to change - * the order of our parameters. * * @param {Shim} shim instance of shim * @param {string} connStr mongo connection string * @param {string} db database name - * @param {object} client mongo client instance */ -function setHostPort(shim, connStr, db, client) { +function setHostPort(shim, connStr, db) { const parts = common.parseAddress(connStr) - // in v3 when running with a cluster of socket connections - // the address is `undefined:undefined`. we will instead attempt - // to get connection details from the client symbol NR_ATTRS - // added in `lib/instrumentation/mongodb/v3-mongo` when a client connects - // with a URL string - if (parts.includes('undefined')) { - try { - const attrs = client[common.NR_ATTRS] - const socket = decodeURIComponent(attrs.split(',')[0].split('mongodb://')[1]) - shim.captureInstanceAttributes('localhost', socket, db) - } catch (err) { - shim.logger.debug(err, 'Could not extract host/port from mongo command') - } - // connected using domain socket but the "host"(e.g: /path/to/mongo-socket-port.sock) - } else if (parts.length && parts[0][0] === '/') { - shim.captureInstanceAttributes('localhost', parts[0], db) - } else { - shim.captureInstanceAttributes(parts[0], parts[1], db) - } + shim.captureInstanceAttributes(parts[0], parts[1], db) } /** @@ -247,13 +219,8 @@ function getInstanceAttributeParameters(shim, mongo) { * @returns {object} db params */ function getParametersFromHosts(hosts, database) { - let [{ host, port }] = hosts - const [{ socketPath }] = hosts + const [{ host, port }] = hosts - if (socketPath) { - port = socketPath - host = 'localhost' - } return new DatastoreParameters({ host, port_path_or_id: port, @@ -284,13 +251,6 @@ function getParametersFromTopology(conf, database) { ;[{ host, port }] = conf.s.options.hosts } - // host is a domain socket. set host as localhost and use the domain - // socket host as the port - if (host && host.endsWith('.sock')) { - port = host - host = 'localhost' - } - return new DatastoreParameters({ host, port_path_or_id: port, diff --git a/lib/instrumentation/mongodb/v3-mongo.js b/lib/instrumentation/mongodb/v3-mongo.js index 4eb4b03a66..14507b8722 100644 --- a/lib/instrumentation/mongodb/v3-mongo.js +++ b/lib/instrumentation/mongodb/v3-mongo.js @@ -11,15 +11,15 @@ const { instrumentBulkOperation, instrumentCollection, instrumentCursor, - instrumentDb, - NR_ATTRS + instrumentDb } = require('./common') /** * parser used to grab the collection and operation * on every mongo operation * - * @param {object} operation + * @param {object} operation mongodb operation + * @returns {object} { operation, collection } parsed operation and collection */ function queryParser(operation) { let collection = this.collectionName || 'unknown' @@ -40,14 +40,11 @@ function queryParser(operation) { * to a Symbol on the MongoClient to be used later to extract the host/port in cases where the topology * is a cluster of domain sockets * - * @param {Shim} shim + * @param {Shim} shim instance of shim * @param {object} mongodb resolved package */ function instrumentClient(shim, mongodb) { - shim.recordOperation(mongodb.MongoClient, 'connect', function wrappedConnect(shim, _, __, args) { - // Add the connection url to the MongoClient to retrieve later in the `lib/instrumentation/mongo/common` - // captureAttributesOnStarted listener - this[NR_ATTRS] = args[0] + shim.recordOperation(mongodb.MongoClient, 'connect', function wrappedConnect(shim) { return new RecorderSpec({ callback: shim.LAST, name: 'connect' }) }) } @@ -61,7 +58,7 @@ function instrumentClient(shim, mongodb) { * as well as sets up a listener for when commands start to properly * add necessary attributes to segments * - * @param {Shim} shim + * @param {Shim} shim instance of shim * @param {object} mongodb resolved package */ module.exports = function instrument(shim, mongodb) { diff --git a/lib/instrumentation/mongodb/v4-mongo.js b/lib/instrumentation/mongodb/v4-mongo.js index 0943685ac2..a36c44fcde 100644 --- a/lib/instrumentation/mongodb/v4-mongo.js +++ b/lib/instrumentation/mongodb/v4-mongo.js @@ -18,7 +18,8 @@ const { * parser used to grab the collection and operation * from a running query * - * @param {object} operation + * @param {object} operation mongodb operation + * @returns {object} { operation, collection } parsed operation and collection */ function queryParser(operation) { let collection = this.collectionName || 'unknown' @@ -39,23 +40,15 @@ function queryParser(operation) { * update host, port and database_name * on segment attributes * - * @param {Shim} shim - * @param {CommandStartedEvent} evnt + * @param {Shim} shim instance of shim + * @param {CommandStartedEvent} evnt mongodb event */ function cmdStartedHandler(shim, evnt) { if (evnt.connectionId) { - let [host, port] = parseAddress(evnt.address) - - // connection via socket get port from 1st host - // socketPath property - // which looks like mongodb:///tmp/mongodb-27017.sock" - if (['undefined'].includes(host, port)) { - host = 'localhost' - const hosts = this.s.options.hosts - if (hosts && hosts.length && hosts[0].socketPath) { - port = hosts[0].socketPath - } - } else if (['127.0.0.1', '::1', '[::1]'].includes(host)) { + const address = parseAddress(evnt.address) + let [host] = address + const [, port] = address + if (['127.0.0.1', '::1', '[::1]'].includes(host)) { host = 'localhost' } @@ -68,7 +61,8 @@ function cmdStartedHandler(shim, evnt) { * enable APM(monitorCommands) and add the * `commandStarted` listener * - * @param {Shim} shim + * @param {Shim} shim instance of shim + * @returns {OperationSpec} spec to capture connect method */ function wrapConnect(shim) { this.monitorCommands = true @@ -82,7 +76,7 @@ function wrapConnect(shim) { * so we can properly update the segment attributes with a more accurate * host/port/database name * - * @param {Shim} shim + * @param {Shim} shim instance of shim * @param {MongoClient} MongoClient reference */ function instrumentMongoClient(shim, MongoClient) { diff --git a/test/versioned/mongodb/collection-common.js b/test/versioned/mongodb/collection-common.js index d339cda2b2..73417bd600 100644 --- a/test/versioned/mongodb/collection-common.js +++ b/test/versioned/mongodb/collection-common.js @@ -184,95 +184,6 @@ function collectionTest(name, run) { }) }) - // The domain socket tests should only be run if there is a domain socket - // to connect to, which only happens if there is a Mongo instance running on - // the same box as these tests. - const domainPath = common.getDomainSocketPath() - const shouldTestDomain = domainPath - t.test('domain socket', { skip: !shouldTestDomain }, function (t) { - t.autoend() - t.beforeEach(async function () { - agent = helper.instrumentMockedAgent() - METRIC_HOST_NAME = agent.config.getHostnameSafe() - METRIC_HOST_PORT = domainPath - - const mongodb = require('mongodb') - - await dropTestCollections(mongodb) - const res = await common.connect(mongodb, domainPath) - client = res.client - db = res.db - collection = db.collection(COLLECTIONS.collection1) - await populate(collection) - }) - - t.afterEach(async function () { - await common.close(client, db) - helper.unloadAgent(agent) - agent = null - }) - - t.test('should have domain socket in metrics', function (t) { - t.notOk(agent.getTransaction(), 'should not have transaction') - helper.runInTransaction(agent, function (transaction) { - transaction.name = common.TRANSACTION_NAME - run(t, collection, function (err, segments, metrics) { - t.error(err) - transaction.end() - const re = new RegExp('^Datastore/instance/MongoDB/' + domainPath) - const badMetrics = Object.keys(agent.metrics._metrics.unscoped).filter(function (m) { - return re.test(m) - }) - t.notOk(badMetrics.length, 'should not use domain path as host name') - common.checkMetrics(t, agent, METRIC_HOST_NAME, METRIC_HOST_PORT, metrics || []) - t.end() - }) - }) - }) - }) - - t.test('domain socket replica set', { skip: !shouldTestDomain }, function (t) { - t.autoend() - t.beforeEach(async function () { - agent = helper.instrumentMockedAgent() - METRIC_HOST_NAME = agent.config.getHostnameSafe() - METRIC_HOST_PORT = domainPath - - const mongodb = require('mongodb') - - await dropTestCollections(mongodb) - const res = await common.connect(mongodb, domainPath) - client = res.client - db = res.db - collection = db.collection(COLLECTIONS.collection1) - await populate(collection) - }) - - t.afterEach(async function () { - await common.close(client, db) - helper.unloadAgent(agent) - agent = null - }) - - t.test('should have domain socket in metrics', function (t) { - t.notOk(agent.getTransaction(), 'should not have transaction') - helper.runInTransaction(agent, function (transaction) { - transaction.name = common.TRANSACTION_NAME - run(t, collection, function (err, segments, metrics) { - t.error(err) - transaction.end() - const re = new RegExp('^Datastore/instance/MongoDB/' + domainPath) - const badMetrics = Object.keys(agent.metrics._metrics.unscoped).filter(function (m) { - return re.test(m) - }) - t.notOk(badMetrics.length, 'should not use domain path as host name') - common.checkMetrics(t, agent, METRIC_HOST_NAME, METRIC_HOST_PORT, metrics || []) - t.end() - }) - }) - }) - }) - // this seems to break in 3.x up to 3.6.0 // I think it is because of this https://jira.mongodb.org/browse/NODE-2452 if (semver.satisfies(pkgVersion, '>=3.6.0')) { diff --git a/test/versioned/mongodb/common.js b/test/versioned/mongodb/common.js index 8b9f61d97a..256d94db16 100644 --- a/test/versioned/mongodb/common.js +++ b/test/versioned/mongodb/common.js @@ -5,7 +5,6 @@ 'use strict' -const fs = require('fs') const mongoPackage = require('mongodb/package.json') const params = require('../../lib/params') const semver = require('semver') @@ -44,7 +43,6 @@ exports.close = function close() { exports.checkMetrics = checkMetrics exports.getHostName = getHostName exports.getPort = getPort -exports.getDomainSocketPath = getDomainSocketPath function connectV2(mongodb, path) { return new Promise((resolve, reject) => { @@ -223,17 +221,6 @@ function checkMetrics(t, agent, host, port, metrics) { }) } -function getDomainSocketPath() { - const files = fs.readdirSync('/tmp') - for (let i = 0; i < files.length; ++i) { - const file = '/tmp/' + files[i] - if (/^\/tmp\/mongodb.*?\.sock$/.test(file)) { - return file - } - } - return null -} - function getMetrics(agent) { return agent.metrics._metrics } diff --git a/test/versioned/mongodb/db-common.js b/test/versioned/mongodb/db-common.js index 221692afdb..ee57ebf320 100644 --- a/test/versioned/mongodb/db-common.js +++ b/test/versioned/mongodb/db-common.js @@ -19,8 +19,6 @@ if (semver.satisfies(common.pkgVersion, '2.2.x')) { function dbTest(name, run) { mongoTest(name, function init(t, agent) { - const LOCALHOST = agent.config.getHostnameSafe() - const domainPath = common.getDomainSocketPath() const mongodb = require('mongodb') let db = null let client = null @@ -65,43 +63,6 @@ function dbTest(name, run) { }) }) }) - - // The domain socket tests should only be run if there is a domain socket - // to connect to, which only happens if there is a Mongo instance running on - // the same box as these tests. - const shouldTestDomain = domainPath - - t.test('domain socket', { skip: !shouldTestDomain }, function (t) { - t.autoend() - t.beforeEach(async function () { - // mongo >= 3.6.9 fails if you try to create an existing collection - // drop before executing tests - if (name === 'createCollection') { - await collectionCommon.dropTestCollections(mongodb) - } - MONGO_HOST = LOCALHOST - MONGO_PORT = domainPath - - const res = await common.connect(mongodb, domainPath) - client = res.client - db = res.db - }) - - t.afterEach(function () { - return common.close(client, db) - }) - - t.test('with transaction', function (t) { - t.notOk(agent.getTransaction(), 'should not have transaction') - helper.runInTransaction(agent, function (transaction) { - run(t, db, function (names, opts = {}) { - verifyMongoSegments(t, agent, transaction, names, opts) - transaction.end() - t.end() - }) - }) - }) - }) }) } diff --git a/test/versioned/mongodb/legacy/cursor.tap.js b/test/versioned/mongodb/legacy/cursor.tap.js index 70623aed14..c9ebee7747 100644 --- a/test/versioned/mongodb/legacy/cursor.tap.js +++ b/test/versioned/mongodb/legacy/cursor.tap.js @@ -15,7 +15,7 @@ const { pkgVersion, STATEMENT_PREFIX, COLLECTIONS } = require('../common') common.test('count', function countTest(t, collection, verify) { collection.find({}).count(function onCount(err, data) { t.notOk(err, 'should not error') - t.equal(data, 30, 'should have correct result') + t.ok(data >= 30, 'should have correct result') verify(null, [`${STATEMENT_PREFIX}/count`, 'Callback: onCount'], ['count']) }) })