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

feat(kafkajs): add kafka cluster id to spans and dsm metrics #4808

Merged
merged 12 commits into from
Oct 25, 2024

Conversation

wconti27
Copy link
Contributor

@wconti27 wconti27 commented Oct 21, 2024

What does this PR do?

Adds kafka cluster id to span tags and dsm metrics AIDM-348

Motivation

Plugin Checklist

Additional Notes

@wconti27 wconti27 requested review from a team as code owners October 21, 2024 16:49
@wconti27 wconti27 marked this pull request as draft October 21, 2024 16:49
Copy link

github-actions bot commented Oct 21, 2024

Overall package size

Self size: 7.6 MB
Deduped: 62.83 MB
No deduping: 63.17 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

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.76%. Comparing base (fd0f570) to head (2f364ef).
Report is 10 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@wconti27 wconti27 changed the title Conti/kafka cluster feat(kafkajs): add kafka cluster id to spans and dsm metrics Oct 21, 2024
@wconti27 wconti27 marked this pull request as ready for review October 21, 2024 19:24
@wconti27 wconti27 self-assigned this Oct 21, 2024
@pr-commenter
Copy link

pr-commenter bot commented Oct 21, 2024

Benchmarks

Benchmark execution time: 2024-10-23 17:38:51

Comparing candidate commit cdaaec5 in PR branch conti/kafka-cluster-id with baseline commit 597d7c5 in branch master.

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

@wconti27 wconti27 requested a review from a team as a code owner October 21, 2024 21:56
if (message !== null && typeof message === 'object') {
message.headers = message.headers || {}
}
return kafkaClusterIdPromise.then((clusterId) => {
Copy link
Collaborator

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.

Copy link
Contributor Author

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()
Copy link
Collaborator

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.

Copy link
Contributor Author

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

@wconti27 wconti27 requested a review from bengl October 24, 2024 16:18
@wconti27 wconti27 merged commit a081659 into master Oct 25, 2024
199 of 200 checks passed
@wconti27 wconti27 deleted the conti/kafka-cluster-id branch October 25, 2024 15:39
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