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

Include send-data missing headers and organize telemetry config variables #3055

Merged
merged 9 commits into from
Jun 2, 2023
Merged

Include send-data missing headers and organize telemetry config variables #3055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fba92b1
Include send-data missing headers
iunanua Apr 27, 2023
72bd2fc
Rename DD_TELEMETRY_DEBUG_ENABLED as DD_TELEMETRY_DIAGNOSTIC_LOG_COLL…
iunanua Apr 27, 2023
e78956d
Move DD_TELEMETRY_HEARTBEAT_INTERVAL to config.js
iunanua Apr 27, 2023
6feb16d
Suggestions
iunanua May 2, 2023
fd744c3
send-data headers capitalization
iunanua May 3, 2023
fcfef2f
Add missing headers
iunanua May 5, 2023
a0d86a8
Minor refactor
iunanua May 10, 2023
ced1feb
Always consider that application has some value
iunanua May 10, 2023
3efca0a
Remove DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED
iunanua May 10, 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
2 changes: 1 addition & 1 deletion packages/dd-trace/src/appsec/iast/telemetry/logs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ function sendLogs () {
}

function isLevelEnabled (level) {
return isLogCollectionEnabled(config) && (level !== 'DEBUG' || config.telemetry.debug)
return isLogCollectionEnabled(config) && level !== 'DEBUG'
}

function isLogCollectionEnabled (config) {
Expand Down
10 changes: 7 additions & 3 deletions packages/dd-trace/src/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,11 @@ class Config {
process.env.DD_TRACE_TELEMETRY_ENABLED,
!process.env.AWS_LAMBDA_FUNCTION_NAME
)
const DD_TELEMETRY_DEBUG_ENABLED = coalesce(
process.env.DD_TELEMETRY_DEBUG_ENABLED,
const DD_TELEMETRY_HEARTBEAT_INTERVAL = process.env.DD_TELEMETRY_HEARTBEAT_INTERVAL
? parseInt(process.env.DD_TELEMETRY_HEARTBEAT_INTERVAL) * 1000
: 60000
const DD_TELEMETRY_DEBUG = coalesce(
process.env.DD_TELEMETRY_DEBUG,
false
)
const DD_TRACE_AGENT_PROTOCOL_VERSION = coalesce(
Expand Down Expand Up @@ -488,8 +491,9 @@ ken|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?)
// Disabled for CI Visibility's agentless
this.telemetry = {
enabled: DD_TRACE_EXPORTER !== 'datadog' && isTrue(DD_TRACE_TELEMETRY_ENABLED),
heartbeatInterval: DD_TELEMETRY_HEARTBEAT_INTERVAL,
logCollection: isTrue(DD_TELEMETRY_LOG_COLLECTION_ENABLED),
debug: isTrue(DD_TELEMETRY_DEBUG_ENABLED)
debug: isTrue(DD_TELEMETRY_DEBUG)
}
this.protocolVersion = DD_TRACE_AGENT_PROTOCOL_VERSION
this.tagsHeaderMaxLength = parseInt(DD_TRACE_X_DATADOG_TAGS_MAX_LENGTH)
Expand Down
11 changes: 5 additions & 6 deletions packages/dd-trace/src/telemetry/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@ const os = require('os')
const dependencies = require('./dependencies')
const { sendData } = require('./send-data')

const HEARTBEAT_INTERVAL = process.env.DD_TELEMETRY_HEARTBEAT_INTERVAL
Copy link
Member

Choose a reason for hiding this comment

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

Why was this moved? It doesn't seem to support programmatic config or be used anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it to group all the telemetry properties reading in one place.

? Number(process.env.DD_TELEMETRY_HEARTBEAT_INTERVAL) * 1000
: 60000

const telemetryStartChannel = dc.channel('datadog:telemetry:start')
const telemetryStopChannel = dc.channel('datadog:telemetry:stop')

Expand All @@ -19,6 +15,7 @@ let pluginManager
let application
let host
let interval
let heartbeatInterval
const sentIntegrations = new Set()

function getIntegrations () {
Expand Down Expand Up @@ -108,7 +105,7 @@ function createHostObject () {
}

function getTelemetryData () {
return { config, application, host, heartbeatInterval: HEARTBEAT_INTERVAL }
return { config, application, host, heartbeatInterval }
}

function start (aConfig, thePluginManager) {
Expand All @@ -119,11 +116,13 @@ function start (aConfig, thePluginManager) {
pluginManager = thePluginManager
application = createAppObject()
host = createHostObject()
heartbeatInterval = config.telemetry.heartbeatInterval

dependencies.start(config, application, host)
sendData(config, application, host, 'app-started', appStarted())
interval = setInterval(() => {
sendData(config, application, host, 'app-heartbeat')
}, HEARTBEAT_INTERVAL)
}, heartbeatInterval)
interval.unref()
process.on('beforeExit', onBeforeExit)

Expand Down
22 changes: 17 additions & 5 deletions packages/dd-trace/src/telemetry/send-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,20 @@
const request = require('../exporters/common/request')

function getHeaders (config, application, reqType) {
const headers = {
'content-type': 'application/json',
'dd-telemetry-api-version': 'v1',
'dd-telemetry-request-type': reqType,
'dd-client-library-language': application.language_name,
Copy link
Member

Choose a reason for hiding this comment

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

Why are we repeating this data that is always available as other headers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

application object, which contains language and version, is included in the request payload but apparently dd-client-library-language and dd-client-library-version headers are mandatory according to the spec.

I gave you a reason in this comment

'dd-client-library-version': application.tracer_version
}
const debug = config.telemetry && config.telemetry.debug
if (debug) {
headers['dd-telemetry-debug-enabled'] = 'true'
}
return headers
}

let seqId = 0

function getPayload (payload) {
Expand All @@ -25,11 +41,7 @@ function sendData (config, application, host, reqType, payload = {}) {
port,
method: 'POST',
path: '/telemetry/proxy/api/v2/apmtelemetry',
headers: {
'content-type': 'application/json',
'dd-telemetry-api-version': 'v1',
'dd-telemetry-request-type': reqType
}
headers: getHeaders(config, application, reqType)
}
const data = JSON.stringify({
api_version: 'v1',
Expand Down
87 changes: 1 addition & 86 deletions packages/dd-trace/test/appsec/iast/iast-log.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,10 @@ const ddBasePath = calculateDDBasePath(__dirname)
const EOL = '\n'

describe('IAST log', () => {
const telemetryDebugConfig = {
config: {
telemetry: {
logCollection: true,
debug: true
}
}
}

const telemetryDefaultConfig = {
config: {
telemetry: {
logCollection: true,
debug: false
logCollection: true
}
}
}
Expand Down Expand Up @@ -77,81 +67,6 @@ describe('IAST log', () => {
telemetryLogs.stop()
})

describe('debug', () => {
it('should call log.debug', () => {
iastLog.debug('debug')

expect(log.debug).to.be.calledOnceWith('debug')
})

it('should call log.debug and not publish msg via telemetry', () => {
iastLog.debugAndPublish('debug')

expect(log.debug).to.be.calledOnceWith('debug')
expect(telemetryLogs.publish).to.not.be.called
})

it('should call log.debug and publish msg via telemetry', () => {
onTelemetryStart(telemetryDebugConfig)

iastLog.debugAndPublish('debug')

expect(log.debug).to.be.calledOnceWith('debug')
expect(telemetryLogs.publish).to.be.calledOnceWith({ message: 'debug', level: 'DEBUG' })
})

it('should chain multiple debug calls', () => {
onTelemetryStart(telemetryDebugConfig)

iastLog.debug('debug').debugAndPublish('debugAndPublish').debug('debug2')

expect(log.debug).to.be.calledThrice
expect(log.debug.getCall(0).args[0]).to.be.eq('debug')
expect(log.debug.getCall(1).args[0]).to.be.eq('debugAndPublish')
expect(log.debug.getCall(2).args[0]).to.be.eq('debug2')
expect(telemetryLogs.publish).to.be.calledOnceWith({ message: 'debugAndPublish', level: 'DEBUG' })
})

it('should chain multiple debug calls', () => {
onTelemetryStart(telemetryDebugConfig)

iastLog.debug('debug')

telemetryLogs.stop()

iastLog.debugAndPublish('debugAndPublish').debug('debug2')
})
})

describe('info', () => {
it('should call log.info', () => {
iastLog.info('info')

expect(log.info).to.be.calledOnceWith('info')
})

it('should call log.info and publish msg via telemetry', () => {
onTelemetryStart(telemetryDebugConfig)

iastLog.infoAndPublish('info')

expect(log.info).to.be.calledOnceWith('info')
expect(telemetryLogs.publish).to.be.calledOnceWith({ message: 'info', level: 'DEBUG' })
})

it('should chain multiple info calls', () => {
onTelemetryStart(telemetryDebugConfig)

iastLog.info('info').infoAndPublish('infoAndPublish').info('info2')

expect(log.info).to.be.calledThrice
expect(log.info.getCall(0).args[0]).to.be.eq('info')
expect(log.info.getCall(1).args[0]).to.be.eq('infoAndPublish')
expect(log.info.getCall(2).args[0]).to.be.eq('info2')
expect(telemetryLogs.publish).to.be.calledOnceWith({ message: 'infoAndPublish', level: 'DEBUG' })
})
})

describe('warn', () => {
it('should call log.warn', () => {
iastLog.warn('warn')
Expand Down
27 changes: 1 addition & 26 deletions packages/dd-trace/test/appsec/iast/telemetry/logs.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,32 +131,7 @@ describe('telemetry logs', () => {
})

describe('sendData', () => {
const app = {}
const host = {}

it('should be called with DEBUG level and error if config.telemetry.debug = true', () => {
const logCollectorAdd = sinon.stub()
const logs = proxyquire('../../../../src/appsec/iast/telemetry/logs', {
'../../../../../diagnostics_channel': dc,
'./log_collector': {
add: logCollectorAdd
}
})
logs.start()

onTelemetryStartMsg.config.telemetry.debug = true
onTelemetryStartMsg.application = app
onTelemetryStartMsg.host = host
onTelemetryStart()(onTelemetryStartMsg)

const error = new Error('test')
const stack = error.stack
logs.publish({ message: error.message, stack_trace: stack, level: 'DEBUG' })

expect(logCollectorAdd).to.be.calledOnceWith(match({ message: 'test', level: 'DEBUG', stack_trace: stack }))
})

it('should be not called with DEBUG level if config.telemetry.debug = false', () => {
it('should be not called with DEBUG level', () => {
const logCollectorAdd = sinon.stub()
const logs = proxyquire('../../../../src/appsec/iast/telemetry/logs', {
'../../../../../diagnostics_channel': dc,
Expand Down
20 changes: 16 additions & 4 deletions packages/dd-trace/test/config.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,10 +786,22 @@ describe('Config', () => {

expect(config.telemetry).to.not.be.undefined
expect(config.telemetry.enabled).to.be.true
expect(config.telemetry.heartbeatInterval).to.eq(60000)
expect(config.telemetry.logCollection).to.be.false
expect(config.telemetry.debug).to.be.false
})

it('should set DD_TELEMETRY_HEARTBEAT_INTERVAL', () => {
const origTelemetryHeartbeatIntervalValue = process.env.DD_TELEMETRY_HEARTBEAT_INTERVAL
process.env.DD_TELEMETRY_HEARTBEAT_INTERVAL = '42'

const config = new Config()

expect(config.telemetry.heartbeatInterval).to.eq(42000)

process.env.DD_TELEMETRY_HEARTBEAT_INTERVAL = origTelemetryHeartbeatIntervalValue
})

it('should not set DD_TRACE_TELEMETRY_ENABLED', () => {
const origTraceTelemetryValue = process.env.DD_TRACE_TELEMETRY_ENABLED
process.env.DD_TRACE_TELEMETRY_ENABLED = 'false'
Expand Down Expand Up @@ -823,15 +835,15 @@ describe('Config', () => {
process.env.DD_IAST_ENABLED = origIastEnabledValue
})

it('should set DD_TELEMETRY_DEBUG_ENABLED', () => {
const origTelemetryDebugValue = process.env.DD_TELEMETRY_DEBUG_ENABLED
process.env.DD_TELEMETRY_DEBUG_ENABLED = 'true'
it('should set DD_TELEMETRY_DEBUG', () => {
const origTelemetryDebugValue = process.env.DD_TELEMETRY_DEBUG
process.env.DD_TELEMETRY_DEBUG = 'true'

const config = new Config()

expect(config.telemetry.debug).to.be.true

process.env.DD_TELEMETRY_DEBUG_ENABLED = origTelemetryDebugValue
process.env.DD_TELEMETRY_DEBUG = origTelemetryDebugValue
})

it('should not set DD_REMOTE_CONFIGURATION_ENABLED if AWS_LAMBDA_FUNCTION_NAME is present', () => {
Expand Down
9 changes: 3 additions & 6 deletions packages/dd-trace/test/telemetry/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('telemetry', () => {
circularObject.child.parent = circularObject

telemetry.start({
telemetry: { enabled: true },
telemetry: { enabled: true, heartbeatInterval: 60000 },
hostname: 'localhost',
port: traceAgent.address().port,
service: 'test service',
Expand Down Expand Up @@ -154,7 +154,7 @@ describe('telemetry', () => {
expect.fail('server should not be called')
}).listen(0, () => {
telemetry.start({
telemetry: { enabled: false },
telemetry: { enabled: false, heartbeatInterval: 60000 },
hostname: 'localhost',
port: server.address().port
})
Expand All @@ -169,8 +169,6 @@ describe('telemetry', () => {

describe('telemetry with interval change', () => {
it('should set the interval correctly', (done) => {
process.env.DD_TELEMETRY_HEARTBEAT_INTERVAL = 12345

const telemetry = proxyquire('../../src/telemetry', {
'../exporters/common/docker': {
id () {
Expand All @@ -190,7 +188,7 @@ describe('telemetry with interval change', () => {
}

telemetry.start({
telemetry: { enabled: true },
telemetry: { enabled: true, heartbeatInterval: 12345000 },
hostname: 'localhost',
port: 8126,
service: 'test service',
Expand All @@ -205,7 +203,6 @@ describe('telemetry with interval change', () => {

process.nextTick(() => {
expect(intervalSetCorrectly).to.be.true
delete process.env.DD_TELEMETRY_HEARTBEAT_INTERVAL
done()
})
})
Expand Down
Loading