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.

Sign up for GitHub

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

Cache integrations - service naming #3056

Merged
merged 8 commits into from
May 23, 2023
Merged

Cache integrations - service naming #3056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
15cc208
switch to folders for versions
Apr 3, 2023
10a372a
switch to kind as direct secondary key for naming schema
Apr 27, 2023
708b223
switch to positional arguments for extra args
Apr 25, 2023
ce382cd
add v0 to all cache integrations
Apr 3, 2023
c1a8c13
add v1 to all cache integrations
Apr 3, 2023
b6f537c
add tests for all cache versions
Apr 3, 2023
ccada49
add v1 tests to cache integrations
Apr 24, 2023
97f7485
Merge branch 'master' into jbertran/service-naming-cache
tlhunter May 22, 2023
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
26 changes: 22 additions & 4 deletions packages/datadog-plugin-ioredis/test/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ const agent = require('../../dd-trace/test/plugins/agent')
const { breakThen, unbreakThen } = require('../../dd-trace/test/plugins/helpers')
const { ERROR_MESSAGE, ERROR_TYPE, ERROR_STACK } = require('../../dd-trace/src/constants')

const Nomenclature = require('../../dd-trace/src/service-naming')
const namingSchema = require('./naming')

describe('Plugin', () => {
let Redis
let redis
Expand All @@ -30,8 +33,8 @@ describe('Plugin', () => {
agent.use(() => {}) // wait for initial info command
agent
.use(traces => {
expect(traces[0][0]).to.have.property('name', 'redis.command')
expect(traces[0][0]).to.have.property('service', 'test-redis')
expect(traces[0][0]).to.have.property('name', namingSchema.outbound.opName)
expect(traces[0][0]).to.have.property('service', namingSchema.outbound.serviceName)
expect(traces[0][0]).to.have.property('resource', 'get')
expect(traces[0][0]).to.have.property('type', 'redis')
expect(traces[0][0].meta).to.have.property('component', 'ioredis')
Expand Down Expand Up @@ -84,8 +87,8 @@ describe('Plugin', () => {
agent.use(() => {}) // wait for initial info command
agent
.use(traces => {
expect(traces[0][0]).to.have.property('name', 'redis.command')
expect(traces[0][0]).to.have.property('service', 'test-redis')
expect(traces[0][0]).to.have.property('name', namingSchema.outbound.opName)
expect(traces[0][0]).to.have.property('service', namingSchema.outbound.serviceName)
expect(traces[0][0]).to.have.property('resource', 'get')
expect(traces[0][0]).to.have.property('type', 'redis')
expect(traces[0][0].meta).to.have.property('db.name', '0')
Expand All @@ -103,6 +106,12 @@ describe('Plugin', () => {

redis.get('foo').catch(done)
})

withNamingSchema(
done => redis.get('foo').catch(done),
() => namingSchema.outbound.opName,
() => namingSchema.outbound.serviceName
)
})

describe('with configuration', () => {
Expand Down Expand Up @@ -135,6 +144,15 @@ describe('Plugin', () => {

redis.get('foo').catch(done)
})

withNamingSchema(
done => redis.get('foo').catch(done),
() => namingSchema.outbound.opName,
() => {
if (Nomenclature.version === 'v0') return 'custom-test'
return 'custom'
}
)
})

describe('with legacy configuration', () => {
Expand Down
14 changes: 14 additions & 0 deletions packages/datadog-plugin-ioredis/test/naming.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const { resolveNaming } = require('../../dd-trace/test/plugins/helpers')

module.exports = resolveNaming({
outbound: {
v0: {
opName: 'redis.command',
serviceName: 'test-redis'
},
v1: {
opName: 'redis.command',
serviceName: 'test'
}
}
})
5 changes: 2 additions & 3 deletions packages/datadog-plugin-memcached/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@ class MemcachedPlugin extends CachePlugin {
start ({ client, server, query }) {
const address = getAddress(client, server, query)

this.startSpan('memcached.command', {
service: this.config.service,
this.startSpan({
service: this.serviceName(this.config, this.system),
resource: query.type,
type: 'memcached',
kind: 'client',
meta: {
'memcached.command': query.command,
'out.host': address[0],
Expand Down
12 changes: 10 additions & 2 deletions packages/datadog-plugin-memcached/test/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ const agent = require('../../dd-trace/test/plugins/agent')
const proxyquire = require('proxyquire').noPreserveCache()
const { ERROR_MESSAGE, ERROR_TYPE, ERROR_STACK } = require('../../dd-trace/src/constants')

const namingSchema = require('./naming')

describe('Plugin', () => {
let Memcached
let memcached
Expand All @@ -28,8 +30,8 @@ describe('Plugin', () => {

agent
.use(traces => {
expect(traces[0][0]).to.have.property('name', 'memcached.command')
expect(traces[0][0]).to.have.property('service', 'test-memcached')
expect(traces[0][0]).to.have.property('name', namingSchema.outbound.opName)
expect(traces[0][0]).to.have.property('service', namingSchema.outbound.serviceName)
expect(traces[0][0]).to.have.property('resource', 'get')
expect(traces[0][0]).to.have.property('type', 'memcached')
expect(traces[0][0].meta).to.have.property('span.kind', 'client')
Expand Down Expand Up @@ -141,6 +143,12 @@ describe('Plugin', () => {
done()
}
})

withNamingSchema(
done => memcached.get('test', err => err && done(err)),
() => namingSchema.outbound.opName,
() => namingSchema.outbound.serviceName
)
})

describe('with configuration', () => {
Expand Down
14 changes: 14 additions & 0 deletions packages/datadog-plugin-memcached/test/naming.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const { resolveNaming } = require('../../dd-trace/test/plugins/helpers')

module.exports = resolveNaming({
outbound: {
v0: {
opName: 'memcached.command',
serviceName: 'test-memcached'
},
v1: {
opName: 'memcached.command',
serviceName: 'test'
}
}
})
15 changes: 2 additions & 13 deletions packages/datadog-plugin-redis/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,10 @@ class RedisPlugin extends CachePlugin {
const normalizedCommand = command.toUpperCase()
if (!this.config.filter(normalizedCommand)) return this.skip()

this.startSpan('redis.command', {
service: getService(this.config, connectionName),
this.startSpan({
resource,
service: this.serviceName(this.config, this.system, connectionName),
type: 'redis',
kind: 'client',
meta: {
'db.type': 'redis',
'db.name': db || '0',
Expand All @@ -33,16 +32,6 @@ class RedisPlugin extends CachePlugin {
}
}

function getService (config, connectionName) {
if (config.splitByInstance && connectionName) {
return config.service
? `${config.service}-${connectionName}`
: connectionName
}

return config.service
}

function formatCommand (command, args) {
if (!args || command === 'AUTH') return command

Expand Down
22 changes: 18 additions & 4 deletions packages/datadog-plugin-redis/test/client.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ const agent = require('../../dd-trace/test/plugins/agent')
const { breakThen, unbreakThen } = require('../../dd-trace/test/plugins/helpers')
const { ERROR_MESSAGE, ERROR_TYPE } = require('../../dd-trace/src/constants')

const namingSchema = require('./naming')

const modules = semver.satisfies(process.versions.node, '>=14')
? ['@node-redis/client', '@redis/client']
: ['@node-redis/client']
Expand Down Expand Up @@ -39,8 +41,8 @@ describe('Plugin', () => {
it('should do automatic instrumentation when using callbacks', async () => {
const promise = agent
.use(traces => {
expect(traces[0][0]).to.have.property('name', 'redis.command')
expect(traces[0][0]).to.have.property('service', 'test-redis')
expect(traces[0][0]).to.have.property('name', namingSchema.outbound.opName)
expect(traces[0][0]).to.have.property('service', namingSchema.outbound.serviceName)
expect(traces[0][0]).to.have.property('resource', 'GET')
expect(traces[0][0]).to.have.property('type', 'redis')
expect(traces[0][0].meta).to.have.property('db.name', '0')
Expand Down Expand Up @@ -76,8 +78,8 @@ describe('Plugin', () => {
it('should work with userland promises', async () => {
const promise = agent
.use(traces => {
expect(traces[0][0]).to.have.property('name', 'redis.command')
expect(traces[0][0]).to.have.property('service', 'test-redis')
expect(traces[0][0]).to.have.property('name', namingSchema.outbound.opName)
expect(traces[0][0]).to.have.property('service', namingSchema.outbound.serviceName)
expect(traces[0][0]).to.have.property('resource', 'GET')
expect(traces[0][0]).to.have.property('type', 'redis')
expect(traces[0][0].meta).to.have.property('db.name', '0')
Expand All @@ -92,6 +94,12 @@ describe('Plugin', () => {
await client.get('foo')
await promise
})

withNamingSchema(
async () => client.get('foo'),
() => namingSchema.outbound.opName,
() => namingSchema.outbound.serviceName
)
})

describe('with configuration', () => {
Expand Down Expand Up @@ -134,6 +142,12 @@ describe('Plugin', () => {
await client.get('foo')
await promise
})

withNamingSchema(
async () => client.get('foo'),
() => namingSchema.outbound.opName,
() => 'custom'
)
})

describe('with blocklist', () => {
Expand Down
18 changes: 16 additions & 2 deletions packages/datadog-plugin-redis/test/legacy.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
const agent = require('../../dd-trace/test/plugins/agent')
const { ERROR_MESSAGE, ERROR_TYPE, ERROR_STACK } = require('../../dd-trace/src/constants')

const namingSchema = require('./naming')

describe('Legacy Plugin', () => {
let redis
let tracer
Expand Down Expand Up @@ -42,8 +44,8 @@ describe('Legacy Plugin', () => {
client.on('error', done)
agent
.use(traces => {
expect(traces[0][0]).to.have.property('name', 'redis.command')
expect(traces[0][0]).to.have.property('service', 'test-redis')
expect(traces[0][0]).to.have.property('name', namingSchema.outbound.opName)
expect(traces[0][0]).to.have.property('service', namingSchema.outbound.serviceName)
expect(traces[0][0]).to.have.property('resource', 'get')
expect(traces[0][0]).to.have.property('type', 'redis')
expect(traces[0][0].meta).to.have.property('db.name', '0')
Expand Down Expand Up @@ -135,6 +137,12 @@ describe('Legacy Plugin', () => {
assertError()
})
})

withNamingSchema(
() => client.get('foo', () => {}),
() => namingSchema.outbound.opName,
() => namingSchema.outbound.serviceName
)
})

describe('with configuration', () => {
Expand Down Expand Up @@ -177,6 +185,12 @@ describe('Legacy Plugin', () => {

client.get('foo', () => {})
})

withNamingSchema(
() => client.get('foo', () => {}),
() => namingSchema.outbound.opName,
() => 'custom'
)
})

describe('with legacy configuration', () => {
Expand Down
14 changes: 14 additions & 0 deletions packages/datadog-plugin-redis/test/naming.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
const { resolveNaming } = require('../../dd-trace/test/plugins/helpers')

module.exports = resolveNaming({
outbound: {
v0: {
opName: 'redis.command',
serviceName: 'test-redis'
},
v1: {
opName: 'redis.command',
serviceName: 'test'
}
}
})
7 changes: 7 additions & 0 deletions packages/dd-trace/src/plugins/cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,13 @@ const StoragePlugin = require('./storage')

class CachePlugin extends StoragePlugin {
static get operation () { return 'command' }

startSpan (options) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part might be a little confusing. Looking across the caching/mysql/tedious PRs: In some classes, startSpan takes two arguments, but in cache related classes, it only takes one?

Or, is the change to make it a single argument going to happen everywhere? There are so many PRs I'm losing track of some of this stuff 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cache plugins get this behaviour generalized because they're all upgraded to respond to the naming schema, so it's possible to do so. Database plugins aren't all yet upgraded, so we can't yet generalize for them, but since it's helping reduce verbosity in the individual plugins I see it as something we want to do when we can.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe as a stop-gap we can rename this method to startSpanImpliedOperation(options) and have it still call super.startSpan? With a TODO to refactor startSpan everywhere to have the same arity?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds better for continuity, will do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That name doesn't really help with understanding IMO. Also, the point of the startSpan method is explicitly to allow overriding it in every plugin.

if (!options.kind) {
options.kind = this.constructor.kind
}
return super.startSpan(this.operationName(), options)
}
}

module.exports = CachePlugin
2 changes: 2 additions & 0 deletions packages/dd-trace/src/plugins/storage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
const ClientPlugin = require('./client')

class StoragePlugin extends ClientPlugin {
static get type () { return 'storage' }

constructor (...args) {
super(...args)

Expand Down
8 changes: 4 additions & 4 deletions packages/dd-trace/src/plugins/tracing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ class TracingPlugin extends Plugin {
return store && store.span
}

serviceName (serviceArgs) {
serviceName (...serviceArgs) {
const { type, id, kind } = this.constructor
return Nomenclature.serviceName(type, kind, id, serviceArgs)
return Nomenclature.serviceName(type, kind, id, ...serviceArgs)
}

operationName (opNameArgs) {
operationName (...opNameArgs) {
const { type, id, kind } = this.constructor
return Nomenclature.opName(type, kind, id, opNameArgs)
return Nomenclature.opName(type, kind, id, ...opNameArgs)
}

configure (config) {
Expand Down
16 changes: 4 additions & 12 deletions packages/dd-trace/src/service-naming/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,5 @@
const { schemaDefinitions } = require('./schemas')

const kindMap = {
messaging: {
client: 'controlPlane',
consumer: 'inbound',
producer: 'outbound'
}
}

class SchemaManager {
constructor () {
this.schemas = schemaDefinitions
Expand All @@ -22,12 +14,12 @@ class SchemaManager {
return this.config.spanAttributeSchema
}

opName (type, kind, plugin, opNameArgs) {
return this.schema.getOpName(type, kindMap[type][kind], plugin, opNameArgs)
opName (type, kind, plugin, ...opNameArgs) {
return this.schema.getOpName(type, kind, plugin, ...opNameArgs)
}

serviceName (type, kind, plugin, serviceNameArgs) {
return this.schema.getServiceName(type, kindMap[type][kind], plugin, this.config.service, serviceNameArgs)
serviceName (type, kind, plugin, ...serviceNameArgs) {
return this.schema.getServiceName(type, kind, plugin, this.config.service, ...serviceNameArgs)
}

configure (config = {}) {
Expand Down
18 changes: 9 additions & 9 deletions packages/dd-trace/src/service-naming/schemas/definition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@ class SchemaDefinition {
this.schema = schema
}

getSchemaItem (type, subType, plugin) {
getSchemaItem (type, kind, plugin) {
const schema = this.schema
if (schema && schema[type] && schema[type][subType] && schema[type][subType][plugin]) {
return schema[type][subType][plugin]
if (schema && schema[type] && schema[type][kind] && schema[type][kind][plugin]) {
return schema[type][kind][plugin]
}
}

getOpName (type, subType, plugin, opNameArgs) {
const item = this.getSchemaItem(type, subType, plugin)
return item.opName(opNameArgs)
getOpName (type, kind, plugin, ...opNameArgs) {
const item = this.getSchemaItem(type, kind, plugin)
return item.opName(...opNameArgs)
}

getServiceName (type, subType, plugin, service, serviceNameArgs) {
const item = this.getSchemaItem(type, subType, plugin)
return item.serviceName(service, serviceNameArgs)
getServiceName (type, kind, plugin, service, ...serviceNameArgs) {
const item = this.getSchemaItem(type, kind, plugin)
return item.serviceName(service, ...serviceNameArgs)
}
}

Expand Down
5 changes: 5 additions & 0 deletions packages/dd-trace/src/service-naming/schemas/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
function identityService (service) {
return service
}

module.exports = { identityService }
Loading