From dcf8ec41d51bf1f9071e05c2349229c5b555a317 Mon Sep 17 00:00:00 2001 From: Ayan Khan Date: Tue, 22 Oct 2024 15:25:26 -0400 Subject: [PATCH] Add Support For Overriding GRPC Error Statuses (#4800) * add support for DD_GRPC_CLIENT_ERROR_STATUSES & DD_GRPC_SERVER_ERROR_STATUSES --- packages/datadog-plugin-grpc/src/client.js | 3 ++ packages/datadog-plugin-grpc/src/server.js | 3 ++ .../datadog-plugin-grpc/test/client.spec.js | 19 ++++++++++- .../datadog-plugin-grpc/test/server.spec.js | 34 ++++++++++++++++++- packages/dd-trace/src/config.js | 28 ++++++++++++++- packages/dd-trace/src/constants.js | 4 ++- packages/dd-trace/src/telemetry/index.js | 4 ++- packages/dd-trace/test/config.spec.js | 33 ++++++++++++++++++ 8 files changed, 123 insertions(+), 5 deletions(-) diff --git a/packages/datadog-plugin-grpc/src/client.js b/packages/datadog-plugin-grpc/src/client.js index ad841aab197..1b130a1f93e 100644 --- a/packages/datadog-plugin-grpc/src/client.js +++ b/packages/datadog-plugin-grpc/src/client.js @@ -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) } diff --git a/packages/datadog-plugin-grpc/src/server.js b/packages/datadog-plugin-grpc/src/server.js index d63164e31c1..0b599a1283d 100644 --- a/packages/datadog-plugin-grpc/src/server.js +++ b/packages/datadog-plugin-grpc/src/server.js @@ -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) } diff --git a/packages/datadog-plugin-grpc/test/client.spec.js b/packages/datadog-plugin-grpc/test/client.spec.js index 38205f1db38..4628fb8a5f6 100644 --- a/packages/datadog-plugin-grpc/test/client.spec.js +++ b/packages/datadog-plugin-grpc/test/client.spec.js @@ -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]) @@ -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 diff --git a/packages/datadog-plugin-grpc/test/server.spec.js b/packages/datadog-plugin-grpc/test/server.spec.js index 2406d087884..cf695840303 100644 --- a/packages/datadog-plugin-grpc/test/server.spec.js +++ b/packages/datadog-plugin-grpc/test/server.spec.js @@ -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'] @@ -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) => { diff --git a/packages/dd-trace/src/config.js b/packages/dd-trace/src/config.js index 1a5eeb61d03..fa502ccb5a2 100644 --- a/packages/dd-trace/src/config.js +++ b/packages/dd-trace/src/config.js @@ -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') @@ -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,}') @@ -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, @@ -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) @@ -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) diff --git a/packages/dd-trace/src/constants.js b/packages/dd-trace/src/constants.js index 61f5b705ddb..a242f717a37 100644 --- a/packages/dd-trace/src/constants.js +++ b/packages/dd-trace/src/constants.js @@ -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] } diff --git a/packages/dd-trace/src/telemetry/index.js b/packages/dd-trace/src/telemetry/index.js index 7988cae5ec2..5df7d6fcae3 100644 --- a/packages/dd-trace/src/telemetry/index.js +++ b/packages/dd-trace/src/telemetry/index.js @@ -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']) diff --git a/packages/dd-trace/test/config.spec.js b/packages/dd-trace/test/config.spec.js index 75a3e51c685..4246167725d 100644 --- a/packages/dd-trace/test/config.spec.js +++ b/packages/dd-trace/test/config.spec.js @@ -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 @@ -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) @@ -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() @@ -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) @@ -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'