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

fix(config): test for completeness of config telemetry #4941

Merged
merged 9 commits into from
Dec 2, 2024

Conversation

bm1549
Copy link
Contributor

@bm1549 bm1549 commented Nov 25, 2024

What does this PR do?

Adds a test that verifies completeness of config telemetry by comparing directly against the files in dd-go's telemetry-payload/static. If a config key does not exist in the rules, it is not processed by our telemetry backend. This is in-line with what we have in dd-trace-dotnet

This PR is open for review, but will not be merged in until the test passes (ie. all telemetry values are handled). We have. a Google sheet tracking proposed decisions (keep/ignore) and normalized config names

The associated change in dd-go is here https://github.com/DataDog/dd-go/pull/160087

Motivation

Telemetry was thought to have been missing from our backend and it wasn't discovered until today. On adding this test, it appears that 65 config telemetry keys are missing

Plugin Checklist

Additional Notes

This is my first PR in this repo and I'm pretty rusty in Node.js - please give me all of your nit picks!

@bm1549 bm1549 requested a review from a team as a code owner November 25, 2024 23:19
Copy link

github-actions bot commented Nov 25, 2024

Overall package size

Self size: 8.18 MB
Deduped: 94.68 MB
No deduping: 95.25 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.2.2 | 29.27 MB | 29.27 MB | | @datadog/native-appsec | 8.3.0 | 19.37 MB | 19.38 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.65 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 3.0.1 | 1.06 MB | 1.46 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 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

@pr-commenter
Copy link

pr-commenter bot commented Nov 26, 2024

Benchmarks

Benchmark execution time: 2024-11-27 20:15:25

Comparing candidate commit f72a08d in PR branch bm1549/config-telemetry-test with baseline commit cd0b922 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 264 metrics, 2 unstable metrics.

.gitignore Outdated Show resolved Hide resolved
"trace.yii_enabled",
"trace.yii_analytics_enabled",
"trace.yii_analytics_sample_rate",
"trace.zendframework_enabled",
Copy link
Member

Choose a reason for hiding this comment

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

It seems weird that frameworks for other platforms are listed in the Node.js codebase. Will all tracers contain an entry for Next.js?

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, because Next.js status isn't sent in the configuration telemetry by default. This file is copied directly from dd-go

@bm1549 bm1549 requested a review from tlhunter November 27, 2024 15:42
tlhunter
tlhunter previously approved these changes Nov 27, 2024
@bm1549 bm1549 merged commit c9be2d4 into master Dec 2, 2024
252 checks passed
@bm1549 bm1549 deleted the bm1549/config-telemetry-test branch December 2, 2024 20:45
rochdev pushed a commit that referenced this pull request Dec 17, 2024
* fix(config): test for completeness of config telemetry

* fully case sensitive checks

* handle blocked key prefixes

* handle aggregation and nodejs specific rules

* Update to latest config rules

* Run eslint

* Apply new config mappings

* revert .gitignore

* Update config_norm_rules.json
@rochdev rochdev mentioned this pull request Dec 17, 2024
rochdev pushed a commit that referenced this pull request Dec 17, 2024
* fix(config): test for completeness of config telemetry

* fully case sensitive checks

* handle blocked key prefixes

* handle aggregation and nodejs specific rules

* Update to latest config rules

* Run eslint

* Apply new config mappings

* revert .gitignore

* Update config_norm_rules.json
@rochdev rochdev mentioned this pull request Dec 17, 2024
rochdev pushed a commit that referenced this pull request Dec 18, 2024
* fix(config): test for completeness of config telemetry

* fully case sensitive checks

* handle blocked key prefixes

* handle aggregation and nodejs specific rules

* Update to latest config rules

* Run eslint

* Apply new config mappings

* revert .gitignore

* Update config_norm_rules.json
rochdev pushed a commit that referenced this pull request Dec 18, 2024
* fix(config): test for completeness of config telemetry

* fully case sensitive checks

* handle blocked key prefixes

* handle aggregation and nodejs specific rules

* Update to latest config rules

* Run eslint

* Apply new config mappings

* revert .gitignore

* Update config_norm_rules.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants