Skip to content

Commit

Permalink
Add Support For Overriding GRPC Error Statuses (#4800)
Browse files Browse the repository at this point in the history
* add support for DD_GRPC_CLIENT_ERROR_STATUSES & DD_GRPC_SERVER_ERROR_STATUSES
  • Loading branch information
khanayan123 authored and rochdev committed Oct 31, 2024
1 parent a6649a4 commit dcf8ec4
Show file tree
Hide file tree
Showing 8 changed files with 123 additions and 5 deletions.
3 changes: 3 additions & 0 deletions packages/datadog-plugin-grpc/src/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ class GrpcClientPlugin extends ClientPlugin {

error ({ span, error }) {
this.addCode(span, error.code)
if (error.code && !this._tracerConfig.grpc.client.error.statuses.includes(error.code)) {
return
}
this.addError(error, span)
}

Expand Down
3 changes: 3 additions & 0 deletions packages/datadog-plugin-grpc/src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ class GrpcServerPlugin extends ServerPlugin {
if (!span) return

this.addCode(span, error.code)
if (error.code && !this._tracerConfig.grpc.server.error.statuses.includes(error.code)) {
return
}
this.addError(error)
}

Expand Down
19 changes: 18 additions & 1 deletion packages/datadog-plugin-grpc/test/client.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ const semver = require('semver')
const Readable = require('stream').Readable
const getService = require('./service')
const loader = require('../../../versions/@grpc/proto-loader').get()
const { ERROR_MESSAGE, ERROR_TYPE, ERROR_STACK } = require('../../dd-trace/src/constants')
const { ERROR_MESSAGE, ERROR_TYPE, ERROR_STACK, GRPC_CLIENT_ERROR_STATUSES } = require('../../dd-trace/src/constants')

const { DD_MAJOR } = require('../../../version')
const nodeMajor = parseInt(process.versions.node.split('.')[0])
Expand Down Expand Up @@ -353,6 +353,23 @@ describe('Plugin', () => {
})
})

it('should ignore errors not set by DD_GRPC_CLIENT_ERROR_STATUSES', async () => {
tracer._tracer._config.grpc.client.error.statuses = [3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]
const client = await buildClient({
getUnary: (_, callback) => callback(new Error('foobar'))
})

client.getUnary({ first: 'foobar' }, () => {})

return agent
.use(traces => {
expect(traces[0][0]).to.have.property('error', 0)
expect(traces[0][0].metrics).to.have.property('grpc.status.code', 2)
tracer._tracer._config.grpc.client.error.statuses =
GRPC_CLIENT_ERROR_STATUSES
})
})

it('should handle protocol errors', async () => {
const definition = loader.loadSync(path.join(__dirname, 'invalid.proto'))
const test = grpc.loadPackageDefinition(definition).test
Expand Down
34 changes: 33 additions & 1 deletion packages/datadog-plugin-grpc/test/server.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const agent = require('../../dd-trace/test/plugins/agent')
const getPort = require('get-port')
const Readable = require('stream').Readable

const { ERROR_MESSAGE, ERROR_TYPE, ERROR_STACK } = require('../../dd-trace/src/constants')
const { ERROR_MESSAGE, ERROR_TYPE, ERROR_STACK, GRPC_SERVER_ERROR_STATUSES } = require('../../dd-trace/src/constants')

const nodeMajor = parseInt(process.versions.node.split('.')[0])
const pkgs = nodeMajor > 14 ? ['@grpc/grpc-js'] : ['grpc', '@grpc/grpc-js']
Expand Down Expand Up @@ -276,6 +276,38 @@ describe('Plugin', () => {
})
})

it('should ignore errors not set by DD_GRPC_SERVER_ERROR_STATUSES', async () => {
tracer._tracer._config.grpc.server.error.statuses = [6, 7, 8, 9, 10, 11, 12, 13]
const client = await buildClient({
getUnary: (_, callback) => {
const metadata = new grpc.Metadata()

metadata.set('extra', 'information')

const error = new Error('foobar')

error.code = grpc.status.NOT_FOUND

const childOf = tracer.scope().active()
const child = tracer.startSpan('child', { childOf })

// Delay trace to ensure auto-cancellation doesn't override the status code.
setTimeout(() => child.finish())

callback(error, {}, metadata)
}
})

client.getUnary({ first: 'foobar' }, () => {})

return agent
.use(traces => {
expect(traces[0][0]).to.have.property('error', 0)
expect(traces[0][0].metrics).to.have.property('grpc.status.code', 5)
tracer._tracer._config.grpc.server.error.statuses = GRPC_SERVER_ERROR_STATUSES
})
})

it('should handle custom errors', async () => {
const client = await buildClient({
getUnary: (_, callback) => {
Expand Down
28 changes: 27 additions & 1 deletion packages/dd-trace/src/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ const { getGitMetadataFromGitProperties, removeUserSensitiveInfo } = require('./
const { updateConfig } = require('./telemetry')
const telemetryMetrics = require('./telemetry/metrics')
const { getIsGCPFunction, getIsAzureFunction } = require('./serverless')
const { ORIGIN_KEY } = require('./constants')
const { ORIGIN_KEY, GRPC_CLIENT_ERROR_STATUSES, GRPC_SERVER_ERROR_STATUSES } = require('./constants')
const { appendRules } = require('./payload-tagging/config')

const tracerMetrics = telemetryMetrics.manager.namespace('tracers')
Expand Down Expand Up @@ -477,6 +477,8 @@ class Config {
this._setValue(defaults, 'flushInterval', 2000)
this._setValue(defaults, 'flushMinSpans', 1000)
this._setValue(defaults, 'gitMetadataEnabled', true)
this._setValue(defaults, 'grpc.client.error.statuses', GRPC_CLIENT_ERROR_STATUSES)
this._setValue(defaults, 'grpc.server.error.statuses', GRPC_SERVER_ERROR_STATUSES)
this._setValue(defaults, 'headerTags', [])
this._setValue(defaults, 'hostname', '127.0.0.1')
this._setValue(defaults, 'iast.cookieFilterPattern', '.{32,}')
Expand Down Expand Up @@ -585,6 +587,8 @@ class Config {
DD_EXPERIMENTAL_API_SECURITY_ENABLED,
DD_EXPERIMENTAL_APPSEC_STANDALONE_ENABLED,
DD_EXPERIMENTAL_PROFILING_ENABLED,
DD_GRPC_CLIENT_ERROR_STATUSES,
DD_GRPC_SERVER_ERROR_STATUSES,
JEST_WORKER_ID,
DD_IAST_COOKIE_FILTER_PATTERN,
DD_IAST_DEDUPLICATION_ENABLED,
Expand Down Expand Up @@ -723,6 +727,8 @@ class Config {
this._setValue(env, 'flushMinSpans', maybeInt(DD_TRACE_PARTIAL_FLUSH_MIN_SPANS))
this._envUnprocessed.flushMinSpans = DD_TRACE_PARTIAL_FLUSH_MIN_SPANS
this._setBoolean(env, 'gitMetadataEnabled', DD_TRACE_GIT_METADATA_ENABLED)
this._setIntegerRangeSet(env, 'grpc.client.error.statuses', DD_GRPC_CLIENT_ERROR_STATUSES)
this._setIntegerRangeSet(env, 'grpc.server.error.statuses', DD_GRPC_SERVER_ERROR_STATUSES)
this._setArray(env, 'headerTags', DD_TRACE_HEADER_TAGS)
this._setString(env, 'hostname', coalesce(DD_AGENT_HOST, DD_TRACE_AGENT_HOSTNAME))
this._setString(env, 'iast.cookieFilterPattern', DD_IAST_COOKIE_FILTER_PATTERN)
Expand Down Expand Up @@ -1170,6 +1176,26 @@ class Config {
}
}

_setIntegerRangeSet (obj, name, value) {
if (value == null) {
return this._setValue(obj, name, null)
}
value = value.split(',')
const result = []

value.forEach(val => {
if (val.includes('-')) {
const [start, end] = val.split('-').map(Number)
for (let i = start; i <= end; i++) {
result.push(i)
}
} else {
result.push(Number(val))
}
})
this._setValue(obj, name, result)
}

_setSamplingRule (obj, name, value) {
if (value == null) {
return this._setValue(obj, name, null)
Expand Down
4 changes: 3 additions & 1 deletion packages/dd-trace/src/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,7 @@ module.exports = {
SCHEMA_ID: 'schema.id',
SCHEMA_TOPIC: 'schema.topic',
SCHEMA_OPERATION: 'schema.operation',
SCHEMA_NAME: 'schema.name'
SCHEMA_NAME: 'schema.name',
GRPC_CLIENT_ERROR_STATUSES: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16],
GRPC_SERVER_ERROR_STATUSES: [2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16]
}
4 changes: 3 additions & 1 deletion packages/dd-trace/src/telemetry/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,9 @@ function updateConfig (changes, config) {
version: 'DD_VERSION',
env: 'DD_ENV',
service: 'DD_SERVICE',
clientIpHeader: 'DD_TRACE_CLIENT_IP_HEADER'
clientIpHeader: 'DD_TRACE_CLIENT_IP_HEADER',
'grpc.client.error.statuses': 'DD_GRPC_CLIENT_ERROR_STATUSES',
'grpc.server.error.statuses': 'DD_GRPC_SERVER_ERROR_STATUSES'
}

const namesNeedFormatting = new Set(['DD_TAGS', 'peerServiceMapping', 'serviceMapping'])
Expand Down
33 changes: 33 additions & 0 deletions packages/dd-trace/test/config.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ require('./setup/tap')
const { expect } = require('chai')
const { readFileSync } = require('fs')
const sinon = require('sinon')
const { GRPC_CLIENT_ERROR_STATUSES, GRPC_SERVER_ERROR_STATUSES } = require('../src/constants')

describe('Config', () => {
let Config
Expand Down Expand Up @@ -225,6 +226,8 @@ describe('Config', () => {
expect(config).to.have.property('traceId128BitGenerationEnabled', true)
expect(config).to.have.property('traceId128BitLoggingEnabled', false)
expect(config).to.have.property('spanAttributeSchema', 'v0')
expect(config.grpc.client.error.statuses).to.deep.equal(GRPC_CLIENT_ERROR_STATUSES)
expect(config.grpc.server.error.statuses).to.deep.equal(GRPC_SERVER_ERROR_STATUSES)
expect(config).to.have.property('spanComputePeerService', false)
expect(config).to.have.property('spanRemoveIntegrationFromService', false)
expect(config).to.have.property('instrumentation_config_id', undefined)
Expand Down Expand Up @@ -498,6 +501,8 @@ describe('Config', () => {
process.env.DD_INSTRUMENTATION_INSTALL_TIME = '1703188212'
process.env.DD_INSTRUMENTATION_CONFIG_ID = 'abcdef123'
process.env.DD_TRACE_ENABLED = 'true'
process.env.DD_GRPC_CLIENT_ERROR_STATUSES = '3,13,400-403'
process.env.DD_GRPC_SERVER_ERROR_STATUSES = '3,13,400-403'

// required if we want to check updates to config.debug and config.logLevel which is fetched from logger
reloadLoggerAndConfig()
Expand All @@ -515,6 +520,8 @@ describe('Config', () => {
expect(config).to.have.property('queryStringObfuscation', '.*')
expect(config).to.have.property('clientIpEnabled', true)
expect(config).to.have.property('clientIpHeader', 'x-true-client-ip')
expect(config.grpc.client.error.statuses).to.deep.equal([3, 13, 400, 401, 402, 403])
expect(config.grpc.server.error.statuses).to.deep.equal([3, 13, 400, 401, 402, 403])
expect(config).to.have.property('runtimeMetrics', true)
expect(config).to.have.property('reportHostname', true)
expect(config).to.have.nested.property('codeOriginForSpans.enabled', true)
Expand Down Expand Up @@ -1001,6 +1008,32 @@ describe('Config', () => {
expect(config).to.have.property('spanAttributeSchema', 'v0')
})

it('should parse integer range sets', () => {
process.env.DD_GRPC_CLIENT_ERROR_STATUSES = '3,13,400-403'
process.env.DD_GRPC_SERVER_ERROR_STATUSES = '3,13,400-403'

let config = new Config()

expect(config.grpc.client.error.statuses).to.deep.equal([3, 13, 400, 401, 402, 403])
expect(config.grpc.server.error.statuses).to.deep.equal([3, 13, 400, 401, 402, 403])

process.env.DD_GRPC_CLIENT_ERROR_STATUSES = '1'
process.env.DD_GRPC_SERVER_ERROR_STATUSES = '1'

config = new Config()

expect(config.grpc.client.error.statuses).to.deep.equal([1])
expect(config.grpc.server.error.statuses).to.deep.equal([1])

process.env.DD_GRPC_CLIENT_ERROR_STATUSES = '2,10,13-15'
process.env.DD_GRPC_SERVER_ERROR_STATUSES = '2,10,13-15'

config = new Config()

expect(config.grpc.client.error.statuses).to.deep.equal([2, 10, 13, 14, 15])
expect(config.grpc.server.error.statuses).to.deep.equal([2, 10, 13, 14, 15])
})

context('peer service tagging', () => {
it('should activate peer service only if explicitly true in v0', () => {
process.env.DD_TRACE_SPAN_ATTRIBUTE_SCHEMA = 'v0'
Expand Down

0 comments on commit dcf8ec4

Please sign in to comment.