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

Add Support For Overriding GRPC Error Statuses #4800

Merged
merged 6 commits into from
Oct 22, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading