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 5 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' || config.telemetry.diagnosticLogCollector)
}

function isLogCollectionEnabled (config) {
Expand Down
15 changes: 12 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 @@ -375,6 +378,10 @@ ken|consumer_?(?:id|key|secret)|sign(?:ed|ature)?|auth(?:entication|orization)?)
process.env.DD_TELEMETRY_LOG_COLLECTION_ENABLED,
DD_IAST_ENABLED
)
const DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED = coalesce(
Copy link
Member

Choose a reason for hiding this comment

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

Why is this no longer enabled by default? How will we know if there is an issue?

Copy link
Member

Choose a reason for hiding this comment

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

Also, using a different term here could be confusing. Why "diagnostic" instead of "debug" like what is used elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At the moment DD_TELEMETRY_LOG_COLLECTION_ENABLED is the property which enables log collection of traces with WARN or ERROR level. It is only used by iast.

And DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED enables log collection of traces with DEBUG or INFO level. It is disabled by default. And really it doesn't have any effect yet as iast is not collecting that kind of traces.

There are more details here

Copy link
Member

Choose a reason for hiding this comment

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

This differs a lot from our current approach. Why not use DEBUG and LOG_LEVEL to be consistent? We'll probably want to normalize all of this properly across the library later on too.

Copy link
Contributor Author

@iunanua iunanua May 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I will discuss this with my team

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, we've decided to remove it until we are interested in collecting DEBUG or INFO traces.

But DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED has other implications besides changing the log level. If true:

  • logs with stack traces sent via telemetry are not redacted and therefore no sensitive information is removed from them.
  • and it is supposed to be valid for 4 hours (to make sure that customers don’t forget)

Copy link
Member

Choose a reason for hiding this comment

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

that's super weird, and I agree with roch :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it has been removed from our implementation but I suppose java and other teams are using it.
wdyt @uurien @rochdev?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah let's make sure it makes sense before implementing it.

process.env.DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED,
false
)

const defaultIastRequestSampling = 30
const iastRequestSampling = coalesce(
Expand Down Expand Up @@ -488,8 +495,10 @@ 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)
diagnosticLogCollection: isTrue(DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_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
24 changes: 19 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,21 @@
const request = require('../exporters/common/request')

function getHeaders (reqType, debug, application) {
const headers = {
'content-type': 'application/json',
'dd-telemetry-api-version': 'v1',
'dd-telemetry-request-type': reqType
}
if (debug) {
headers['dd-telemetry-debug-enabled'] = 'true'
}
if (application) {
Copy link
Member

Choose a reason for hiding this comment

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

How could this be missing? These are 2 process-level information that should always be available.

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 guess they are added by the agent, so there is no need to add them in the tracer.

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'm not sure :S, let me check it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, they are not added by the agent.

And they are mandatory headers according to the spec

I know the application data is included among the request data but it seems that the headers help to identify the request even if the backend fails to parse the request payload. For example to identify a particular library version and language, sending requests that cannot be parsed.

So it is useful for the backend that the tracer includes them.

Copy link
Member

Choose a reason for hiding this comment

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

It's fine if they're needed, but in which case would the condition be false? There would always be an application, and this it seems that this would always be true, so why the if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because sendData method doesn't guarantee that application will always have a value and getHeaders method accesses some application properties.
I felt safer with the if.
Removed

headers['dd-client-library-language'] = application.language_name
headers['dd-client-library-version'] = application.tracer_version
}
return headers
}

let seqId = 0

function getPayload (payload) {
Expand All @@ -19,17 +36,14 @@ function sendData (config, application, host, reqType, payload = {}) {
url
} = config

const debug = config.telemetry && config.telemetry.debug
const options = {
url,
hostname,
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(reqType, debug, application)
}
const data = JSON.stringify({
api_version: 'v1',
Expand Down
4 changes: 2 additions & 2 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 @@ -10,7 +10,7 @@ describe('IAST log', () => {
config: {
telemetry: {
logCollection: true,
debug: true
diagnosticLogCollector: true
}
}
}
Expand All @@ -19,7 +19,7 @@ describe('IAST log', () => {
config: {
telemetry: {
logCollection: true,
debug: false
diagnosticLogCollector: false
}
}
}
Expand Down
6 changes: 3 additions & 3 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 @@ -134,7 +134,7 @@ describe('telemetry logs', () => {
const app = {}
const host = {}

it('should be called with DEBUG level and error if config.telemetry.debug = true', () => {
it('should be called with DEBUG level and error if config.telemetry.diagnosticLogCollector = true', () => {
const logCollectorAdd = sinon.stub()
const logs = proxyquire('../../../../src/appsec/iast/telemetry/logs', {
'../../../../../diagnostics_channel': dc,
Expand All @@ -144,7 +144,7 @@ describe('telemetry logs', () => {
})
logs.start()

onTelemetryStartMsg.config.telemetry.debug = true
onTelemetryStartMsg.config.telemetry.diagnosticLogCollector = true
onTelemetryStartMsg.application = app
onTelemetryStartMsg.host = host
onTelemetryStart()(onTelemetryStartMsg)
Expand All @@ -156,7 +156,7 @@ describe('telemetry logs', () => {
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 if config.telemetry.diagnosticLogCollector = false', () => {
const logCollectorAdd = sinon.stub()
const logs = proxyquire('../../../../src/appsec/iast/telemetry/logs', {
'../../../../../diagnostics_channel': dc,
Expand Down
32 changes: 28 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,23 @@ 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.diagnosticLogCollection).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 +836,26 @@ 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_DIAGNOSTIC_LOG_COLLECTION_ENABLED = true', () => {
const origDiagnosticLogCollectionValue = process.env.DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED
process.env.DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED = 'true'

const config = new Config()

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

process.env.DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED = origDiagnosticLogCollectionValue
})

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
55 changes: 51 additions & 4 deletions packages/dd-trace/test/telemetry/send-data.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ require('../setup/tap')

const proxyquire = require('proxyquire')
describe('sendData', () => {
const application = {
language_name: 'nodejs',
tracer_version: 'version'
}

let sendDataModule
let request
beforeEach(() => {
Expand All @@ -12,8 +17,14 @@ describe('sendData', () => {
'../exporters/common/request': request
})
})

it('should call to request (TCP)', () => {
sendDataModule.sendData({ hostname: '', port: '12345', tags: { 'runtime-id': '123' } }, 'test', 'test', 'req-type')
sendDataModule.sendData({
hostname: '',
port: '12345',
tags: { 'runtime-id': '123' }
}, application, 'test', 'req-type')

expect(request).to.have.been.calledOnce
const options = request.getCall(0).args[1]

Expand All @@ -23,15 +34,22 @@ describe('sendData', () => {
headers: {
'content-type': 'application/json',
'dd-telemetry-api-version': 'v1',
'dd-telemetry-request-type': 'req-type'
'dd-telemetry-request-type': 'req-type',
'dd-client-library-language': application.language_name,
'dd-client-library-version': application.tracer_version
},
url: undefined,
hostname: '',
port: '12345'
})
})

it('should call to request (UDP)', () => {
sendDataModule.sendData({ url: 'unix:/foo/bar/baz', tags: { 'runtime-id': '123' } }, 'test', 'test', 'req-type')
sendDataModule.sendData({
url: 'unix:/foo/bar/baz',
tags: { 'runtime-id': '123' }
}, application, 'test', 'req-type')

expect(request).to.have.been.calledOnce
const options = request.getCall(0).args[1]

Expand All @@ -41,14 +59,43 @@ describe('sendData', () => {
headers: {
'content-type': 'application/json',
'dd-telemetry-api-version': 'v1',
'dd-telemetry-request-type': 'req-type'
'dd-telemetry-request-type': 'req-type',
'dd-client-library-language': application.language_name,
'dd-client-library-version': application.tracer_version
},
url: 'unix:/foo/bar/baz',
hostname: undefined,
port: undefined
})
})

it('should add debug header if DD_TELEMETRY_DEBUG is present', () => {
sendDataModule.sendData({
url: '/test',
tags: { 'runtime-id': '123' },
telemetry: { debug: true }
}, application, 'test', 'req-type')

expect(request).to.have.been.calledOnce
const options = request.getCall(0).args[1]

expect(options).to.deep.equal({
method: 'POST',
path: '/telemetry/proxy/api/v2/apmtelemetry',
headers: {
'content-type': 'application/json',
'dd-telemetry-api-version': 'v1',
'dd-telemetry-request-type': 'req-type',
'dd-telemetry-debug-enabled': 'true',
'dd-client-library-language': application.language_name,
'dd-client-library-version': application.tracer_version
},
url: '/test',
hostname: undefined,
port: undefined
})
})

it('should remove not wanted properties from a payload with object type', () => {
const payload = {
message: 'test',
Expand Down