-
Notifications
You must be signed in to change notification settings - Fork 310
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
feat(kafkajs): add kafka cluster id to spans and dsm metrics #4808
Conversation
Overall package sizeSelf size: 7.6 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.2.1 | 19.18 MB | 19.19 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 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 | 2.0.0 | 898.77 kB | 1.3 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 | | 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 | | 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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4808 +/- ##
===========================================
+ Coverage 68.58% 91.76% +23.18%
===========================================
Files 12 136 +124
Lines 818 4664 +3846
===========================================
+ Hits 561 4280 +3719
- Misses 257 384 +127 ☔ View full report in Codecov by Sentry. |
if (message !== null && typeof message === 'object') { | ||
message.headers = message.headers || {} | ||
} | ||
return kafkaClusterIdPromise.then((clusterId) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the promise has already resolved, it's unnecessary overhead to call .then()
on it. Instead you can store the clusterId in its own variable (or just use this._ddKafkaClusterId
), and if that variable is not set, then you can call .then()
on the promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I changed the code to check if the returned data is a promise, if it is, it calls .then()
, otherwise continues as normal without calling .then()
await admin.connect() | ||
const clusterInfo = await admin.describeCluster() | ||
const clusterId = clusterInfo?.clusterId | ||
await admin.disconnect() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can actually disconnect as soon as you've gotten clusterInfo
, and you don't have to await
it, since you don't need it to be disconnected to proceed.
That said, all async/await
syntax in this file should be converted to promise chains, for performance reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, converted this to use promise chains and the admin disconnects as soon as we've got clusterInfo
Adds Kafka cluster ID to KafkaJS spans and DSM metrics
Adds Kafka cluster ID to KafkaJS spans and DSM metrics
Adds Kafka cluster ID to KafkaJS spans and DSM metrics
Adds Kafka cluster ID to KafkaJS spans and DSM metrics
Adds Kafka cluster ID to KafkaJS spans and DSM metrics
What does this PR do?
Adds kafka cluster id to span tags and dsm metrics AIDM-348
Motivation
Plugin Checklist
Additional Notes