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

refactor: Updated shim classes to no longer construct specs. #2096

Merged
merged 4 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion THIRD_PARTY_NOTICES.md
Original file line number Diff line number Diff line change
Expand Up @@ -712,7 +712,7 @@ This product includes source derived from [@newrelic/aws-sdk](https://github.com

### @newrelic/koa

This product includes source derived from [@newrelic/koa](https://github.com/newrelic/node-newrelic-koa) ([v9.0.0](https://github.com/newrelic/node-newrelic-koa/tree/v9.0.0)), distributed under the [Apache-2.0 License](https://github.com/newrelic/node-newrelic-koa/blob/v9.0.0/LICENSE):
This product includes source derived from [@newrelic/koa](https://github.com/newrelic/node-newrelic-koa) ([v9.1.0](https://github.com/newrelic/node-newrelic-koa/tree/v9.1.0)), distributed under the [Apache-2.0 License](https://github.com/newrelic/node-newrelic-koa/blob/v9.1.0/LICENSE):

```
Apache License
Expand Down
4 changes: 3 additions & 1 deletion lib/instrumentation/cassandra-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ module.exports = function initialize(_agent, cassandra, _moduleName, shim) {
shim.recordOperation(
ClientProto,
['connect', 'shutdown'],
new OperationSpec({ callback: shim.LAST })
function operationSpec(shim, _fn, name) {
return new OperationSpec({ callback: shim.LAST, name })
}
)

if (semver.satisfies(cassandraVersion, '>=4.4.0')) {
Expand Down
66 changes: 33 additions & 33 deletions lib/instrumentation/mongodb/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,9 @@ common.NR_ATTRS = Symbol('NR_ATTRS')
common.instrumentCursor = function instrumentCursor(shim, Cursor) {
if (Cursor && Cursor.prototype) {
const proto = Cursor.prototype
for (let i = 0; i < CURSOR_OPS.length; i++) {
shim.recordQuery(proto, CURSOR_OPS[i], common.makeQueryDescFunc(shim, CURSOR_OPS[i]))
}

shim.recordQuery(proto, 'each', common.makeQueryDescFunc(shim, 'each'))
shim.recordOperation(proto, 'pipe', new OperationSpec({ opaque: true }))
shim.recordQuery(proto, CURSOR_OPS, common.makeQueryDescFunc)
shim.recordQuery(proto, 'each', common.makeQueryDescFunc)
shim.recordOperation(proto, 'pipe', new OperationSpec({ opaque: true, name: 'pipe' }))
}
}

Expand All @@ -42,9 +39,7 @@ common.instrumentCursor = function instrumentCursor(shim, Cursor) {
common.instrumentCollection = function instrumentCollection(shim, Collection) {
if (Collection && Collection.prototype) {
const proto = Collection.prototype
for (let i = 0; i < COLLECTION_OPS.length; i++) {
shim.recordQuery(proto, COLLECTION_OPS[i], common.makeQueryDescFunc(shim, COLLECTION_OPS[i]))
}
shim.recordQuery(proto, COLLECTION_OPS, common.makeQueryDescFunc)
}
}

Expand Down Expand Up @@ -72,43 +67,48 @@ common.instrumentBulkOperation = function instrumentBulkOperation(shim, BulkOper
common.instrumentDb = function instrumentDb(shim, Db) {
if (Db && Db.prototype) {
const proto = Db.prototype
shim.recordOperation(proto, DB_OPS, function makeOperationDescFunc(shim, _fn, methodName) {
return new OperationSpec({
callback: shim.LAST,
opaque: true,
promise: true,
name: methodName
})
})
// link to client.connect(removed in v4.0)
shim.recordOperation(
proto,
DB_OPS,
new OperationSpec({ callback: shim.LAST, opaque: true, promise: true })
Db,
'connect',
new OperationSpec({ callback: shim.LAST, promise: true, name: 'connect' })
)
// link to client.connect(removed in v4.0)
shim.recordOperation(Db, 'connect', new OperationSpec({ callback: shim.LAST, promise: true }))
}
}

/**
* Sets up the desc for all instrumented query methods
*
* @param {Shim} shim instance of shim
* @param {string} methodName name of method getting executed
* @returns {object} query spec
* @param {Function} fn function getting instrumented
* @param _fn
* @param {string} methodName name of function
* @returns {QuerySpec} query spec
*/
common.makeQueryDescFunc = function makeQueryDescFunc(shim, methodName) {
common.makeQueryDescFunc = function makeQueryDescFunc(shim, _fn, methodName) {
if (methodName === 'each') {
return function eachDescFunc() {
const parameters = getInstanceAttributeParameters(shim, this)
return new QuerySpec({ query: methodName, parameters, rowCallback: shim.LAST, opaque: true })
}
}

return function queryDescFunc() {
// segment name does not actually use query string
// method name is set as query so the query parser has access to the op name
const parameters = getInstanceAttributeParameters(shim, this)
return new QuerySpec({
query: methodName,
parameters,
promise: true,
callback: shim.LAST,
opaque: true
})
return new QuerySpec({ query: methodName, parameters, rowCallback: shim.LAST, opaque: true })
}

// segment name does not actually use query string
// method name is set as query so the query parser has access to the op name
const parameters = getInstanceAttributeParameters(shim, this)
return new QuerySpec({
query: methodName,
parameters,
promise: true,
callback: shim.LAST,
opaque: true
})
}

/**
Expand Down
16 changes: 10 additions & 6 deletions lib/instrumentation/mongodb/v2-mongo.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ module.exports = function instrument(shim, mongodb) {
const recordDesc = {
Gridstore: {
isQuery: false,
makeDescFunc: function makeGridDesc(opName) {
makeDescFunc: function makeGridDesc(shim, fn, opName) {
return new OperationSpec({ name: 'GridFS-' + opName, callback: shim.LAST })
}
},
Expand All @@ -50,8 +50,8 @@ module.exports = function instrument(shim, mongodb) {
Collection: { isQuery: true, makeDescFunc: makeQueryDescFunc },
Db: {
isQuery: false,
makeDescFunc: function makeDbDesc() {
return new OperationSpec({ callback: shim.LAST })
makeDescFunc: function makeDbDesc(shim, fn, method) {
return new OperationSpec({ callback: shim.LAST, name: method })
}
}
}
Expand Down Expand Up @@ -95,10 +95,10 @@ module.exports = function instrument(shim, mongodb) {
const { isQuery, makeDescFunc } = recordDesc[objectName]
const proto = object.prototype
if (isQuery) {
shim.recordQuery(proto, method, makeDescFunc(shim, method))
shim.recordQuery(proto, method, makeDescFunc)
} else if (isQuery === false) {
// could be unset
shim.recordOperation(proto, method, makeDescFunc(shim, method))
shim.recordOperation(proto, method, makeDescFunc)
} else {
shim.logger.trace('No wrapping method found for %s', objectName)
}
Expand All @@ -108,7 +108,11 @@ module.exports = function instrument(shim, mongodb) {
// the cursor object implements Readable stream and internally calls nextObject on
// each read, in which case we do not want to record each nextObject() call
if (/Cursor$/.test(objectName)) {
shim.recordOperation(object.prototype, 'pipe', new OperationSpec({ opaque: true }))
shim.recordOperation(
object.prototype,
'pipe',
new OperationSpec({ opaque: true, name: 'pipe' })
)
}
}
}
2 changes: 1 addition & 1 deletion lib/instrumentation/mongodb/v3-mongo.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ function instrumentClient(shim, mongodb) {
// Add the connection url to the MongoClient to retrieve later in the `lib/instrumentation/mongo/common`
// captureAttributesOnStarted listener
this[NR_ATTRS] = args[0]
return new RecorderSpec({ callback: shim.LAST })
return new RecorderSpec({ callback: shim.LAST, name: 'connect' })
})
}

Expand Down
2 changes: 1 addition & 1 deletion lib/instrumentation/mongodb/v4-mongo.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ function cmdStartedHandler(shim, evnt) {
function wrapConnect(shim) {
this.monitorCommands = true
this.on('commandStarted', cmdStartedHandler.bind(this, shim))
return new OperationSpec({ callback: shim.LAST })
return new OperationSpec({ callback: shim.LAST, name: 'connect' })
}

/**
Expand Down
6 changes: 3 additions & 3 deletions lib/instrumentation/pg.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
'use strict'

const { nrEsmProxy } = require('../symbols')
const { RecorderSpec, QuerySpec } = require('../shim/specs')
const { RecorderSpec, QuerySpec, ClassWrapSpec } = require('../shim/specs')
const DatastoreParameters = require('../shim/specs/params/datastore')

function getQuery(shim, original, name, args) {
Expand Down Expand Up @@ -58,12 +58,12 @@ module.exports = function initialize(agent, pgsql, moduleName, shim) {
// wrapping for native
function instrumentPGNative(pg) {
shim.wrapReturn(pg, 'Client', clientFactoryWrapper)
shim.wrapClass(pg, 'Pool', { post: poolPostConstructor, es6: true })
shim.wrapClass(pg, 'Pool', new ClassWrapSpec({ post: poolPostConstructor, es6: true }))
}

function poolPostConstructor(shim) {
if (!shim.isWrapped(this.Client)) {
shim.wrapClass(this, 'Client', clientPostConstructor)
shim.wrapClass(this, 'Client', new ClassWrapSpec({ post: clientPostConstructor }))
}
}

Expand Down
33 changes: 18 additions & 15 deletions lib/shim/datastore-shim.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,8 @@ const ParsedStatement = require('../db/parsed-statement')
const Shim = require('./shim')
const urltils = require('../util/urltils')
const util = require('util')
const {
params: { DatastoreParameters }
} = require('./specs/')
const specs = require('./specs')
const { DatastoreParameters } = specs.params

/**
* An enumeration of well-known datastores so that new instrumentations can use
Expand Down Expand Up @@ -263,18 +262,28 @@ function recordOperation(nodule, properties, opSpec) {
opSpec = properties
properties = null
}

// TODO: not a fan of this. people should always pass in some sort of spec
if (!opSpec) {
opSpec = Object.create(null)
}

return this.record(nodule, properties, function operationRecorder(shim, fn, fnName, args) {
shim.logger.trace('Recording datastore operation "%s"', fnName)

const segDesc = _getSpec.call(this, { spec: opSpec, shim, fn, fnName, args })
const segDesc = _getSpec.call(this, {
spec: opSpec,
shim,
fn,
fnName,
args
})

// Adjust the segment name with the metric prefix and add a recorder.
if (!hasOwnProperty(segDesc, 'record') || segDesc.record !== false) {
segDesc.name = shim._metrics.OPERATION + segDesc.name
if (!segDesc?.name?.startsWith(shim._metrics.OPERATION)) {
segDesc.name = shim._metrics.OPERATION + segDesc.name
}
segDesc.recorder = _recordOperationMetrics.bind(shim)
}

Expand Down Expand Up @@ -534,6 +543,7 @@ function _recordQuery(suffix, nodule, properties, querySpec) {
querySpec = properties
properties = null
}

if (!querySpec) {
this.logger.debug('Missing query spec for recordQuery, not wrapping.')
return nodule
Expand Down Expand Up @@ -579,20 +589,13 @@ function _recordQuery(suffix, nodule, properties, querySpec) {
*
*/
function _getSpec({ spec, shim, fn, fnName, args }) {
let dsSpec = null
let dsSpec = spec
if (shim.isFunction(spec)) {
dsSpec = spec.call(this, shim, fn, fnName, args)
} else {
dsSpec = { ...spec }
}

dsSpec.name = dsSpec.name || fnName || 'other'

const parameters = _normalizeParameters.call(shim, dsSpec.parameters)
return {
...dsSpec,
parameters
}
dsSpec.parameters = _normalizeParameters.call(shim, dsSpec.parameters)
return dsSpec
}

/**
Expand Down
3 changes: 1 addition & 2 deletions lib/shim/message-shim/consume.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,8 @@ function updateSpecFromArgs({ shim, fn, fnName, args, spec }) {
let msgDesc = null
if (shim.isFunction(spec)) {
msgDesc = spec.call(this, shim, fn, fnName, args)
msgDesc = new specs.MessageSpec(msgDesc)
} else {
msgDesc = new specs.MessageSpec(spec)
msgDesc = spec
const destIdx = shim.normalizeIndex(args.length, spec.destinationName)
if (destIdx !== null) {
msgDesc.destinationName = args[destIdx]
Expand Down