refactor(mongodb): Removed instrumentation that handles connecting via unix domain socket. (#2129)

Author: bizob2828

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,

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) {

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) {

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')) {

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
 }

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()
-          })
-        })
-      })
-    })
   })
 }
 

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'])
   })
 })