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

iunanua · 2023-04-27T08:31:53Z

What does this PR do?

Include dd-client-library-language and dd-client-library-version headers
Move DD_TELEMETRY_HEARTBEAT_INTERVAL to config.js

Motivation

Some system-tests will check them

Plugin Checklist

Additional Notes

github-actions · 2023-04-27T08:32:46Z

Overall package size

Self size: 4.04 MB
Deduped: 64.79 MB
No deduping: 64.84 MB

Dependency sizes

name	version	self size	total size
@datadog/pprof	2.2.1	14.24 MB	15.12 MB
@datadog/native-iast-taint-tracking	1.4.0	14.89 MB	14.9 MB
@datadog/native-appsec	3.1.0	13.31 MB	13.32 MB
@datadog/native-metrics	1.6.0	7.88 MB	7.89 MB
protobufjs	7.1.2	2.76 MB	6.55 MB
@datadog/native-iast-rewriter	2.0.1	2.09 MB	2.1 MB
opentracing	0.14.7	194.81 kB	194.81 kB
semver	7.3.8	88.2 kB	118.6 kB
@datadog/sketches-js	2.1.0	109.9 kB	109.9 kB
lodash.sortby	4.7.0	75.76 kB	75.76 kB
lru-cache	7.14.0	74.95 kB	74.95 kB
ipaddr.js	2.0.1	59.52 kB	59.52 kB
ignore	5.2.0	48.87 kB	48.87 kB
import-in-the-middle	1.3.5	34.34 kB	38.81 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
retry	0.10.1	27.44 kB	27.44 kB
lodash.uniq	4.5.0	25.01 kB	25.01 kB
limiter	1.1.5	23.17 kB	23.17 kB
lodash.kebabcase	4.1.1	17.75 kB	17.75 kB
lodash.pick	4.4.0	16.33 kB	16.33 kB
node-abort-controller	3.0.1	14.33 kB	14.33 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
diagnostics_channel	1.1.0	7.07 kB	7.07 kB
path-to-regexp	0.1.7	6.78 kB	6.78 kB
koalas	1.0.2	6.47 kB	6.47 kB
methods	1.1.2	5.29 kB	5.29 kB
module-details-from-path	1.0.3	4.47 kB	4.47 kB

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

codecov · 2023-04-27T08:35:21Z

Codecov Report

Merging #3055 (3efca0a) into master (124a892) will increase coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3055      +/-   ##
==========================================
+ Coverage   86.84%   87.16%   +0.32%     
==========================================
  Files         323      318       -5     
  Lines       11649    11614      -35     
  Branches       33       33              
==========================================
+ Hits        10116    10123       +7     
+ Misses       1533     1491      -42

Impacted Files	Coverage Δ
...ackages/dd-trace/src/appsec/iast/telemetry/logs.js	`97.50% <100.00%> (ø)`
packages/dd-trace/src/config.js	`98.85% <100.00%> (-0.02%)`	⬇️
packages/dd-trace/src/telemetry/index.js	`95.16% <100.00%> (ø)`
packages/dd-trace/src/telemetry/send-data.js	`100.00% <100.00%> (ø)`

... and 44 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pr-commenter · 2023-04-27T08:35:26Z

Benchmarks

Comparing candidate commit ced1feb in PR branch igor/send-data-missing-headers with baseline commit 124a892 in branch master.

Found 0 performance improvements and 11 performance regressions! Performance is the same for 654 metrics, 43 unstable metrics.

scenario:log-with-error-14

🟥 execution_time [+9.021ms; +10.301ms] or [+5.709%; +6.518%]

scenario:log-with-debug-14

🟥 execution_time [+8.770ms; +9.886ms] or [+5.547%; +6.252%]

scenario:log-skip-log-14

🟥 cpu_user_time [+7.282ms; +10.476ms] or [+5.329%; +7.666%]
🟥 execution_time [+8.051ms; +9.321ms] or [+5.086%; +5.888%]

scenario:startup-with-tracer-14

🟥 execution_time [+8.602ms; +9.530ms] or [+5.487%; +6.079%]

scenario:log-with-debug-16

🟥 execution_time [+8.195ms; +9.026ms] or [+5.060%; +5.573%]

scenario:log-with-error-16

🟥 execution_time [+8.404ms; +9.368ms] or [+5.183%; +5.777%]

scenario:log-skip-log-16

🟥 execution_time [+8.199ms; +9.021ms] or [+5.050%; +5.556%]

scenario:log-without-log-16

🟥 execution_time [+8.466ms; +9.656ms] or [+5.201%; +5.932%]

scenario:startup-with-tracer-16

🟥 execution_time [+8.133ms; +8.946ms] or [+5.084%; +5.593%]

scenario:startup-with-tracer-18

🟥 execution_time [+8.525ms; +9.246ms] or [+5.127%; +5.561%]

packages/dd-trace/src/telemetry/send-data.js

packages/dd-trace/src/config.js

packages/dd-trace/src/telemetry/send-data.js

packages/dd-trace/src/config.js

packages/dd-trace/src/telemetry/index.js

packages/dd-trace/test/config.spec.js

packages/dd-trace/test/telemetry/send-data.spec.js

…ECTION_ENABLED

simon-id

LGTM but maybe get an approval from the tracing team too i guess ? 👍

packages/dd-trace/src/appsec/iast/telemetry/logs.js

rochdev · 2023-05-04T14:32:04Z

packages/dd-trace/src/telemetry/index.js

@@ -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


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

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

rochdev · 2023-05-04T14:33:24Z

packages/dd-trace/src/config.js

@@ -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(


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

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

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

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.

I will discuss this with my team

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)

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

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

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

rochdev · 2023-05-04T14:34:35Z

packages/dd-trace/src/telemetry/send-data.js

+  if (debug) {
+    headers['dd-telemetry-debug-enabled'] = 'true'
+  }
+  if (application) {


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

I guess they are added by the agent, so there is no need to add them in the tracer.

I'm not sure :S, let me check it again.

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.

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?

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

packages/dd-trace/src/config.js

rochdev · 2023-05-23T17:35:35Z

packages/dd-trace/src/telemetry/send-data.js

+    'content-type': 'application/json',
+    'dd-telemetry-api-version': 'v1',
+    'dd-telemetry-request-type': reqType,
+    'dd-client-library-language': application.language_name,


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

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

…bles (#3055) * Include send-data missing headers * Rename DD_TELEMETRY_DEBUG_ENABLED as DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED * Move DD_TELEMETRY_HEARTBEAT_INTERVAL to config.js * Remove DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED

iunanua added the semver-patch label Apr 27, 2023

iunanua force-pushed the igor/send-data-missing-headers branch from 808def9 to 85213de Compare April 27, 2023 08:35

iunanua marked this pull request as ready for review April 27, 2023 10:13

iunanua requested a review from a team as a code owner April 27, 2023 10:13

simon-id reviewed Apr 27, 2023

View reviewed changes

packages/dd-trace/src/telemetry/send-data.js Outdated Show resolved Hide resolved

iunanua marked this pull request as draft April 27, 2023 14:18

iunanua marked this pull request as ready for review April 27, 2023 14:40

iunanua requested a review from a team as a code owner April 27, 2023 14:40

iunanua changed the title ~~Include send-data missing headers~~ Include send-data missing headers and organize telemetry config variables Apr 27, 2023

iunanua force-pushed the igor/send-data-missing-headers branch 2 times, most recently from 2afea60 to 60ac59b Compare April 27, 2023 15:45

Qard previously approved these changes Apr 27, 2023

View reviewed changes

rochdev requested changes Apr 27, 2023

View reviewed changes

packages/dd-trace/src/config.js Show resolved Hide resolved

iunanua requested a review from simon-id April 28, 2023 07:37

rochdev previously approved these changes Apr 28, 2023

View reviewed changes

uurien reviewed May 2, 2023

View reviewed changes

packages/dd-trace/src/telemetry/send-data.js Outdated Show resolved Hide resolved

simon-id reviewed May 2, 2023

View reviewed changes

iunanua dismissed stale reviews from rochdev and Qard via 6a8e990 May 2, 2023 12:14

iunanua force-pushed the igor/send-data-missing-headers branch from c950657 to cb62f2d Compare May 3, 2023 11:07

iunanua added 5 commits May 3, 2023 16:06

Include send-data missing headers

fba92b1

Rename DD_TELEMETRY_DEBUG_ENABLED as DD_TELEMETRY_DIAGNOSTIC_LOG_COLL…

72bd2fc

…ECTION_ENABLED

Move DD_TELEMETRY_HEARTBEAT_INTERVAL to config.js

e78956d

Suggestions

6feb16d

send-data headers capitalization

fd744c3

iunanua force-pushed the igor/send-data-missing-headers branch from cb62f2d to fd744c3 Compare May 3, 2023 14:10

simon-id previously approved these changes May 4, 2023

View reviewed changes

iunanua requested review from Qard and rochdev May 4, 2023 14:17

rochdev requested changes May 4, 2023

View reviewed changes

iunanua dismissed simon-id’s stale review via f20f97b May 4, 2023 16:39

iunanua changed the title ~~Include send-data missing headers and organize telemetry config variables~~ Organize telemetry config variables May 4, 2023

Add missing headers

fcfef2f

iunanua force-pushed the igor/send-data-missing-headers branch from e9584af to fcfef2f Compare May 5, 2023 10:36

iunanua changed the title ~~Organize telemetry config variables~~ Include send-data missing headers and organize telemetry config variables May 5, 2023

iunanua mentioned this pull request May 8, 2023

Report IAST metrics via telemetry #2805

Merged

1 task

iunanua requested a review from rochdev May 8, 2023 20:00

iunanua added 3 commits May 10, 2023 09:30

Minor refactor

a0d86a8

Always consider that application has some value

ced1feb

Remove DD_TELEMETRY_DIAGNOSTIC_LOG_COLLECTION_ENABLED

3efca0a

uurien approved these changes May 22, 2023

View reviewed changes

rochdev reviewed May 23, 2023

View reviewed changes

rochdev approved these changes Jun 1, 2023

View reviewed changes

iunanua merged commit cf892cc into master Jun 2, 2023

This was referenced Jun 8, 2023

v2.35.1 #3219

Merged

v3.22.1 #3220

Merged

v4.1.1 #3221

Merged

iunanua mentioned this pull request Sep 19, 2023

Include the extra services found in spans in the remote config client request payloads [APPSEC-9476] #3635

Merged

6 tasks

iunanua deleted the igor/send-data-missing-headers branch December 19, 2023 08:29

erikvanbrakel mentioned this pull request Jul 5, 2024

[DEVX-3565] add neo4j plugin for v4.41 mazedesignhq/dd-trace-js#15

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

iunanua commented Apr 27, 2023 •

edited

Loading

github-actions bot commented Apr 27, 2023 •

edited

Loading

codecov bot commented Apr 27, 2023 •

edited

Loading

pr-commenter bot commented Apr 27, 2023 •

edited

Loading

simon-id left a comment •

edited

Loading

rochdev May 4, 2023

iunanua May 4, 2023

rochdev May 4, 2023

rochdev May 4, 2023

iunanua May 4, 2023

rochdev May 9, 2023

iunanua May 10, 2023 •

edited

Loading

iunanua May 10, 2023

simon-id May 16, 2023

iunanua May 16, 2023

rochdev May 23, 2023

rochdev May 4, 2023

iunanua May 4, 2023

iunanua May 4, 2023

iunanua May 5, 2023

rochdev May 9, 2023

iunanua May 10, 2023

rochdev May 23, 2023

iunanua May 24, 2023

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

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

Conversation

iunanua commented Apr 27, 2023 • edited Loading

What does this PR do?

Motivation

Plugin Checklist

Additional Notes

github-actions bot commented Apr 27, 2023 • edited Loading

Overall package size

Dependency sizes

codecov bot commented Apr 27, 2023 • edited Loading

Codecov Report

pr-commenter bot commented Apr 27, 2023 • edited Loading

Benchmarks

scenario:log-with-error-14

scenario:log-with-debug-14

scenario:log-skip-log-14

scenario:startup-with-tracer-14

scenario:log-with-debug-16

scenario:log-with-error-16

scenario:log-skip-log-16

scenario:log-without-log-16

scenario:startup-with-tracer-16

scenario:startup-with-tracer-18

simon-id left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iunanua May 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iunanua commented Apr 27, 2023 •

edited

Loading

github-actions bot commented Apr 27, 2023 •

edited

Loading

codecov bot commented Apr 27, 2023 •

edited

Loading

pr-commenter bot commented Apr 27, 2023 •

edited

Loading

simon-id left a comment •

edited

Loading

iunanua May 10, 2023 •

edited

Loading