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

chore: Refactor specs into classes for easier code navigation #2004

Merged
merged 20 commits into from Feb 13, 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: 2 additions & 0 deletions .eslintrc.js
Expand Up @@ -11,6 +11,8 @@ module.exports = {
rules: {
'consistent-return': 'off',
'jsdoc/require-jsdoc': 'off',
'jsdoc/tag-lines': 'off',
jsumners-nr marked this conversation as resolved.
Show resolved Hide resolved
'jsdoc/check-types': 'off',
bizob2828 marked this conversation as resolved.
Show resolved Hide resolved
'jsdoc/no-undefined-types': [
'warn',
{
Expand Down
2 changes: 1 addition & 1 deletion THIRD_PARTY_NOTICES.md
Expand Up @@ -2431,7 +2431,7 @@ THE SOFTWARE.

### eslint-plugin-jsdoc

This product includes source derived from [eslint-plugin-jsdoc](https://github.com/gajus/eslint-plugin-jsdoc) ([v39.9.1](https://github.com/gajus/eslint-plugin-jsdoc/tree/v39.9.1)), distributed under the [BSD-3-Clause License](https://github.com/gajus/eslint-plugin-jsdoc/blob/v39.9.1/LICENSE):
This product includes source derived from [eslint-plugin-jsdoc](https://github.com/gajus/eslint-plugin-jsdoc) ([v48.0.5](https://github.com/gajus/eslint-plugin-jsdoc/tree/v48.0.5)), distributed under the [BSD-3-Clause License](https://github.com/gajus/eslint-plugin-jsdoc/blob/v48.0.5/LICENSE):

```
Copyright (c) 2018, Gajus Kuizinas (http://gajus.com/)
Expand Down
104 changes: 67 additions & 37 deletions lib/instrumentation/express.js
Expand Up @@ -5,9 +5,12 @@

'use strict'

const { MiddlewareSpec, MiddlewareMounterSpec } = require('../../lib/shim/specs')
const { MIDDLEWARE_TYPE_NAMES } = require('../../lib/shim/webframework-shim/common')

/**
* Express middleware generates traces where middleware are considered siblings
* (ended on 'next' invocation) and not nested. Middlware are nested below the
* (ended on 'next' invocation) and not nested. Middleware are nested below the
* routers they are mounted to.
*/

Expand All @@ -32,15 +35,23 @@
function wrapExpress4(shim, express) {
// Wrap `use` and `route` which are hung off `Router` directly, not on a
// prototype.
shim.wrapMiddlewareMounter(express.Router, 'use', {
route: shim.FIRST,
wrapper: wrapMiddleware
})

shim.wrapMiddlewareMounter(express.application, 'use', {
route: shim.FIRST,
wrapper: wrapMiddleware
})
shim.wrapMiddlewareMounter(
express.Router,
'use',
new MiddlewareMounterSpec({
route: shim.FIRST,
wrapper: wrapMiddleware
})
)

shim.wrapMiddlewareMounter(
express.application,
'use',
new MiddlewareMounterSpec({
route: shim.FIRST,
wrapper: wrapMiddleware
})
)

shim.wrap(express.Router, 'route', function wrapRoute(shim, fn) {
if (!shim.isFunction(fn)) {
Expand All @@ -61,13 +72,17 @@
// This wraps a 'done' function but not a traditional 'next' function. This allows
// the route to stay on the stack for middleware nesting after the router.
// The segment will be automatically ended by the http/https instrumentation.
shim.recordMiddleware(layer, 'handle', {
type: shim.ROUTE,
req: shim.FIRST,
next: shim.LAST,
matchArity: true,
route: route.path
})
shim.recordMiddleware(
layer,
'handle',
new MiddlewareSpec({
type: shim.ROUTE,
req: shim.FIRST,
next: shim.LAST,
matchArity: true,
route: route.path
})
)
}
return route
}
Expand All @@ -76,11 +91,14 @@
shim.wrapMiddlewareMounter(express.Router, 'param', {
route: shim.FIRST,
wrapper: function wrapParamware(shim, middleware, fnName, route) {
return shim.recordParamware(middleware, {
name: route,
req: shim.FIRST,
next: shim.THIRD
})
return shim.recordParamware(
middleware,
new MiddlewareSpec({
name: route,
req: shim.FIRST,
next: shim.THIRD
})
)
}
})

Expand All @@ -97,21 +115,33 @@
shim.wrapMiddlewareMounter(express.Router.prototype, 'param', {
route: shim.FIRST,
wrapper: function wrapParamware(shim, middleware, fnName, route) {
return shim.recordParamware(middleware, {
name: route,
req: shim.FIRST,
next: shim.THIRD
})
return shim.recordParamware(
middleware,
new MiddlewareSpec({
name: route,
req: shim.FIRST,
next: shim.THIRD,
type: MIDDLEWARE_TYPE_NAMES.PARAMWARE
})
)

Check warning on line 126 in lib/instrumentation/express.js

View check run for this annotation

Codecov / codecov/patch

lib/instrumentation/express.js#L118-L126

Added lines #L118 - L126 were not covered by tests
}
})
shim.wrapMiddlewareMounter(express.Router.prototype, 'use', {
route: shim.FIRST,
wrapper: wrapMiddleware
})
shim.wrapMiddlewareMounter(express.application, 'use', {
route: shim.FIRST,
wrapper: wrapMiddleware
})
shim.wrapMiddlewareMounter(
express.Router.prototype,
'use',
new MiddlewareMounterSpec({
route: shim.FIRST,
wrapper: wrapMiddleware
})
)
shim.wrapMiddlewareMounter(
express.application,
'use',
new MiddlewareMounterSpec({
route: shim.FIRST,
wrapper: wrapMiddleware
})
)

// NOTE: Do not wrap application route methods in Express 3, they all just
// forward their arguments to the router.
Expand Down Expand Up @@ -153,12 +183,12 @@

function wrapMiddleware(shim, middleware, name, route) {
let method = null
const spec = {
const spec = new MiddlewareSpec({
route: route,
type: shim.MIDDLEWARE,
matchArity: true,
req: shim.FIRST
}
})

if (middleware.lazyrouter) {
method = 'handle'
Expand Down
10 changes: 6 additions & 4 deletions lib/instrumentation/fastify/spec-builders.js
Expand Up @@ -5,6 +5,8 @@

'use strict'

const { MiddlewareSpec } = require('../../shim/specs')

/**
* Retrieves the IncomingMessage from a Fastify request. Depending on the
* context of this function it either exists on `request.raw` or just `request`
Expand Down Expand Up @@ -55,7 +57,7 @@ const getParamsFromFastifyRequest = (shim, fn, fnName, args) => {
* @returns {object} spec for Fastify route handler
*/
function buildMiddlewareSpecForRouteHandler(shim, path) {
return {
return new MiddlewareSpec({
/**
* A function where we can wrap next, reply send, etc. methods
*
Expand Down Expand Up @@ -93,7 +95,7 @@ function buildMiddlewareSpecForRouteHandler(shim, path) {
params: getParamsFromFastifyRequest,
req: getRequestFromFastify,
route: path
}
})
}

/**
Expand All @@ -105,14 +107,14 @@ function buildMiddlewareSpecForRouteHandler(shim, path) {
* @returns {object} spec for Fastify middleware
*/
function buildMiddlewareSpecForMiddlewareFunction(shim, name, route) {
return {
return new MiddlewareSpec({
name,
route,
next: shim.LAST,
params: getParamsFromFastifyRequest,
req: getRequestFromFastify,
type: shim.MIDDLEWARE
}
})
}

module.exports = {
Expand Down
6 changes: 5 additions & 1 deletion lib/instrumentation/grpc-js/grpc.js
Expand Up @@ -7,6 +7,7 @@

const recordExternal = require('../../metrics/recorders/http_external')
const recordHttp = require('../../metrics/recorders/http')
const specs = require('../../shim/specs')
const { DESTINATIONS } = require('../../config/attribute-filter')
const DESTINATION = DESTINATIONS.TRANS_EVENT | DESTINATIONS.ERROR_EVENT
const semver = require('semver')
Expand Down Expand Up @@ -146,7 +147,10 @@ function wrapRegister(shim, original) {
return original.apply(this, arguments)
}

args[1] = shim.bindCreateTransaction(instrumentedHandler, { type: shim.WEB })
args[1] = shim.bindCreateTransaction(
instrumentedHandler,
new specs.TransactionSpec({ type: shim.WEB })
)

return original.apply(this, args)

Expand Down
9 changes: 5 additions & 4 deletions lib/instrumentation/openai.js
Expand Up @@ -12,6 +12,7 @@ const {
LlmErrorMessage
} = require('../../lib/llm-events/openai')
const LlmTrackedIds = require('../../lib/llm-events/tracked-ids')
const { RecorderSpec } = require('../../lib/shim/specs')

const MIN_VERSION = '4.0.0'
const MIN_STREAM_VERSION = '4.12.2'
Expand Down Expand Up @@ -267,7 +268,7 @@ module.exports = function initialize(agent, openai, moduleName, shim) {
return
}

return {
return new RecorderSpec({
name: OPENAI.COMPLETION,
promise: true,
// eslint-disable-next-line max-params
Expand All @@ -286,7 +287,7 @@ module.exports = function initialize(agent, openai, moduleName, shim) {

segment.transaction.trace.attributes.addAttribute(DESTINATIONS.TRANS_EVENT, 'llm', true)
}
}
})
}
)

Expand All @@ -299,7 +300,7 @@ module.exports = function initialize(agent, openai, moduleName, shim) {
'create',
function wrapEmbeddingCreate(shim, embeddingCreate, name, args) {
const [request] = args
return {
return new RecorderSpec({
name: OPENAI.EMBEDDING,
promise: true,
// eslint-disable-next-line max-params
Expand Down Expand Up @@ -335,7 +336,7 @@ module.exports = function initialize(agent, openai, moduleName, shim) {
delete response.api_key
delete response.headers
}
}
})
}
)
}
13 changes: 7 additions & 6 deletions lib/instrumentation/pg.js
Expand Up @@ -6,6 +6,7 @@
'use strict'

const { nrEsmProxy } = require('../symbols')
const specs = require('../shim/specs')

function getQuery(shim, original, name, args) {
const config = args[0]
Expand Down Expand Up @@ -35,22 +36,22 @@ module.exports = function initialize(agent, pgsql, moduleName, shim) {
// pg supports event based Client.query when a Query object is passed in,
// and works similarly in pg version <7.0.0
if (typeof queryArgs[0] === 'string') {
return {
return new specs.QuerySpec({
callback: shim.LAST,
query: getQuery,
promise: true,
parameters: getInstanceParameters(shim, this),
internal: false
}
})
}

return {
return new specs.QuerySpec({
callback: shim.LAST,
query: getQuery,
stream: 'row',
parameters: getInstanceParameters(shim, this),
internal: false
}
})
}

// wrapping for native
Expand All @@ -73,10 +74,10 @@ module.exports = function initialize(agent, pgsql, moduleName, shim) {
shim.recordQuery(this, 'query', wrapJSClientQuery)

shim.record(this, 'connect', function pgConnectNamer() {
return {
return new specs.QuerySpec({
name: 'connect',
callback: shim.LAST
}
})
})
}

Expand Down
3 changes: 2 additions & 1 deletion lib/serverless/aws-lambda.js
Expand Up @@ -13,6 +13,7 @@ const recordBackground = require('../metrics/recorders/other')
const recordWeb = require('../metrics/recorders/http')
const TransactionShim = require('../shim/transaction-shim')
const urltils = require('../util/urltils')
const specs = require('../shim/specs')

// CONSTANTS
const ATTR_DEST = require('../config/attribute-filter').DESTINATIONS
Expand Down Expand Up @@ -121,7 +122,7 @@ class AwsLambda {
this.wrapFatal()
}

return shim.bindCreateTransaction(wrappedHandler, { type: shim.BG })
return shim.bindCreateTransaction(wrappedHandler, new specs.TransactionSpec({ type: shim.BG }))

function wrappedHandler() {
const args = shim.argsToArray.apply(shim, arguments)
Expand Down