Support Kafka-clients 3.8+ #7626
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; |
There was a problem hiding this comment.
⚪ Code Quality Violation
Suggested change
| if (this == o) return true; | |
| if (this == o) {return true}; |
single if statement should be wrapped in a brace (...read more)
Omitting braces {} is valid in multiple statements, such as, for loops, if statements, and while loops. However, enforcing the use of control braces throughout your codebase will make the code more consistent and can make it easier to add statements in the future.
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; |
There was a problem hiding this comment.
⚪ Code Quality Violation
Suggested change
| if (o == null || getClass() != o.getClass()) return false; | |
| if (o == null || getClass() != o.getClass()) {return false}; |
single if statement should be wrapped in a brace (...read more)
Omitting braces {} is valid in multiple statements, such as, for loops, if statements, and while loops. However, enforcing the use of control braces throughout your codebase will make the code more consistent and can make it easier to add statements in the future.
Comment on lines
81
to
85
| protected KafkaDecorator(String spanKind, CharSequence spanType, String serviceName) { | ||
| this.spanKind = spanKind; | ||
| this.spanType = spanType; | ||
| this.serviceName = serviceName; | ||
| } |
There was a problem hiding this comment.
⚪ Code Quality Violation
Consider adding super() or this() to your constructor (...read more)
In Java, it is suggested to call super() in an extended class. This rule will report a violation if both a call to super() and an overloaded constructor are absent.
Kafka / producer-benchmarkParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 2 metrics, 0 unstable metrics.
See unchanged results
|
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 53 metrics, 10 unstable metrics. Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.41.0-SNAPSHOT~02132f2834, baseline=1.41.0-SNAPSHOT~a80d13e96c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.081 s) : 0, 1081419
Total [baseline] (8.653 s) : 0, 8653208
Agent [candidate] (1.079 s) : 0, 1078578
Total [candidate] (8.61 s) : 0, 8610311
section iast
Agent [baseline] (1.206 s) : 0, 1205974
Total [baseline] (9.201 s) : 0, 9201317
Agent [candidate] (1.21 s) : 0, 1209527
Total [candidate] (9.147 s) : 0, 9147074
section iast_HARDCODED_SECRET_DISABLED
Agent [baseline] (1.201 s) : 0, 1200511
Total [baseline] (9.149 s) : 0, 9148645
Agent [candidate] (1.217 s) : 0, 1216898
Total [candidate] (9.122 s) : 0, 9122026
section iast_TELEMETRY_OFF
Agent [baseline] (1.207 s) : 0, 1206650
Total [baseline] (9.104 s) : 0, 9103580
Agent [candidate] (1.197 s) : 0, 1197097
Total [candidate] (9.101 s) : 0, 9100944
gantt
title insecure-bank - break down per module: candidate=1.41.0-SNAPSHOT~02132f2834, baseline=1.41.0-SNAPSHOT~a80d13e96c
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (687.997 ms) : 0, 687997
BytebuddyAgent [candidate] (689.235 ms) : 0, 689235
GlobalTracer [baseline] (316.801 ms) : 0, 316801
GlobalTracer [candidate] (312.817 ms) : 0, 312817
AppSec [baseline] (54.501 ms) : 0, 54501
AppSec [candidate] (54.263 ms) : 0, 54263
Remote Config [baseline] (673.782 µs) : 0, 674
Remote Config [candidate] (670.554 µs) : 0, 671
Telemetry [baseline] (7.634 ms) : 0, 7634
Telemetry [candidate] (7.706 ms) : 0, 7706
section iast
BytebuddyAgent [baseline] (802.257 ms) : 0, 802257
BytebuddyAgent [candidate] (806.101 ms) : 0, 806101
GlobalTracer [baseline] (303.147 ms) : 0, 303147
GlobalTracer [candidate] (302.33 ms) : 0, 302330
AppSec [baseline] (57.658 ms) : 0, 57658
AppSec [candidate] (57.751 ms) : 0, 57751
IAST [baseline] (21.339 ms) : 0, 21339
IAST [candidate] (21.627 ms) : 0, 21627
Remote Config [baseline] (608.271 µs) : 0, 608
Remote Config [candidate] (621.156 µs) : 0, 621
Telemetry [baseline] (7.109 ms) : 0, 7109
Telemetry [candidate] (7.167 ms) : 0, 7167
section iast_HARDCODED_SECRET_DISABLED
BytebuddyAgent [baseline] (797.842 ms) : 0, 797842
BytebuddyAgent [candidate] (811.246 ms) : 0, 811246
GlobalTracer [baseline] (302.141 ms) : 0, 302141
GlobalTracer [candidate] (304.06 ms) : 0, 304060
AppSec [baseline] (55.93 ms) : 0, 55930
AppSec [candidate] (57.144 ms) : 0, 57144
IAST [baseline] (23.06 ms) : 0, 23060
IAST [candidate] (22.562 ms) : 0, 22562
Remote Config [baseline] (611.954 µs) : 0, 612
Remote Config [candidate] (631.589 µs) : 0, 632
Telemetry [baseline] (7.114 ms) : 0, 7114
Telemetry [candidate] (7.259 ms) : 0, 7259
section iast_TELEMETRY_OFF
BytebuddyAgent [baseline] (802.144 ms) : 0, 802144
BytebuddyAgent [candidate] (796.524 ms) : 0, 796524
GlobalTracer [baseline] (304.137 ms) : 0, 304137
GlobalTracer [candidate] (300.571 ms) : 0, 300571
AppSec [baseline] (54.742 ms) : 0, 54742
AppSec [candidate] (54.381 ms) : 0, 54381
IAST [baseline] (24.065 ms) : 0, 24065
IAST [candidate] (24.267 ms) : 0, 24267
Remote Config [baseline] (621.012 µs) : 0, 621
Remote Config [candidate] (615.947 µs) : 0, 616
Telemetry [baseline] (7.05 ms) : 0, 7050
Telemetry [candidate] (6.942 ms) : 0, 6942
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.41.0-SNAPSHOT~02132f2834, baseline=1.41.0-SNAPSHOT~a80d13e96c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.075 s) : 0, 1074725
Total [baseline] (10.461 s) : 0, 10460686
Agent [candidate] (1.072 s) : 0, 1071765
Total [candidate] (10.414 s) : 0, 10414227
section appsec
Agent [baseline] (1.213 s) : 0, 1212527
Total [baseline] (10.642 s) : 0, 10641826
Agent [candidate] (1.215 s) : 0, 1214551
Total [candidate] (10.69 s) : 0, 10690237
section iast
Agent [baseline] (1.21 s) : 0, 1210396
Total [baseline] (10.941 s) : 0, 10940783
Agent [candidate] (1.2 s) : 0, 1199863
Total [candidate] (10.932 s) : 0, 10932434
section profiling
Agent [baseline] (1.27 s) : 0, 1270394
Total [baseline] (10.697 s) : 0, 10697209
Agent [candidate] (1.28 s) : 0, 1280308
Total [candidate] (10.738 s) : 0, 10737906
gantt
title petclinic - break down per module: candidate=1.41.0-SNAPSHOT~02132f2834, baseline=1.41.0-SNAPSHOT~a80d13e96c
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (684.294 ms) : 0, 684294
BytebuddyAgent [candidate] (684.946 ms) : 0, 684946
GlobalTracer [baseline] (314.433 ms) : 0, 314433
GlobalTracer [candidate] (310.906 ms) : 0, 310906
AppSec [baseline] (54.116 ms) : 0, 54116
AppSec [candidate] (53.89 ms) : 0, 53890
Remote Config [baseline] (661.675 µs) : 0, 662
Remote Config [candidate] (657.374 µs) : 0, 657
Telemetry [baseline] (7.473 ms) : 0, 7473
Telemetry [candidate] (7.579 ms) : 0, 7579
section appsec
BytebuddyAgent [baseline] (702.547 ms) : 0, 702547
BytebuddyAgent [candidate] (706.8 ms) : 0, 706800
GlobalTracer [baseline] (312.292 ms) : 0, 312292
GlobalTracer [candidate] (309.739 ms) : 0, 309739
AppSec [baseline] (162.636 ms) : 0, 162636
AppSec [candidate] (163.819 ms) : 0, 163819
Remote Config [baseline] (642.653 µs) : 0, 643
Remote Config [candidate] (641.388 µs) : 0, 641
Telemetry [baseline] (9.21 ms) : 0, 9210
Telemetry [candidate] (8.264 ms) : 0, 8264
IAST [baseline] (22.813 ms) : 0, 22813
IAST [candidate] (22.859 ms) : 0, 22859
section iast
BytebuddyAgent [baseline] (805.386 ms) : 0, 805386
BytebuddyAgent [candidate] (799.942 ms) : 0, 799942
GlobalTracer [baseline] (304.645 ms) : 0, 304645
GlobalTracer [candidate] (299.859 ms) : 0, 299859
AppSec [baseline] (56.772 ms) : 0, 56772
AppSec [candidate] (54.915 ms) : 0, 54915
Remote Config [baseline] (593.629 µs) : 0, 594
Remote Config [candidate] (614.935 µs) : 0, 615
Telemetry [baseline] (7.047 ms) : 0, 7047
Telemetry [candidate] (7.072 ms) : 0, 7072
IAST [baseline] (22.108 ms) : 0, 22108
IAST [candidate] (23.696 ms) : 0, 23696
section profiling
BytebuddyAgent [baseline] (675.538 ms) : 0, 675538
BytebuddyAgent [candidate] (681.779 ms) : 0, 681779
GlobalTracer [baseline] (396.809 ms) : 0, 396809
GlobalTracer [candidate] (398.321 ms) : 0, 398321
AppSec [baseline] (54.576 ms) : 0, 54576
AppSec [candidate] (55.384 ms) : 0, 55384
Remote Config [baseline] (667.337 µs) : 0, 667
Remote Config [candidate] (669.424 µs) : 0, 669
Telemetry [baseline] (7.437 ms) : 0, 7437
Telemetry [candidate] (7.613 ms) : 0, 7613
ProfilingAgent [baseline] (96.601 ms) : 0, 96601
ProfilingAgent [candidate] (97.504 ms) : 0, 97504
Profiling [baseline] (96.625 ms) : 0, 96625
Profiling [candidate] (97.528 ms) : 0, 97528
LoadDacapo |
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; |
There was a problem hiding this comment.
⚪ Code Quality Violation
Suggested change
| if (this == o) return true; | |
| if (this == o) {return true}; |
single if statement should be wrapped in a brace (...read more)
Omitting braces {} is valid in multiple statements, such as, for loops, if statements, and while loops. However, enforcing the use of control braces throughout your codebase will make the code more consistent and can make it easier to add statements in the future.
Comment on lines
81
to
85
| protected KafkaDecorator(String spanKind, CharSequence spanType, String serviceName) { | ||
| this.spanKind = spanKind; | ||
| this.spanType = spanType; | ||
| this.serviceName = serviceName; | ||
| } |
There was a problem hiding this comment.
⚪ Code Quality Violation
Consider adding super() or this() to your constructor (...read more)
In Java, it is suggested to call super() in an extended class. This rule will report a violation if both a call to super() and an overloaded constructor are absent.
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; |
There was a problem hiding this comment.
⚪ Code Quality Violation
Suggested change
| if (o == null || getClass() != o.getClass()) return false; | |
| if (o == null || getClass() != o.getClass()) {return false}; |
single if statement should be wrapped in a brace (...read more)
Omitting braces {} is valid in multiple statements, such as, for loops, if statements, and while loops. However, enforcing the use of control braces throughout your codebase will make the code more consistent and can make it easier to add statements in the future.
Comment on lines
+81
to
+85
| protected KafkaDecorator(String spanKind, CharSequence spanType, String serviceName) { | ||
| this.spanKind = spanKind; | ||
| this.spanType = spanType; | ||
| this.serviceName = serviceName; | ||
| } |
There was a problem hiding this comment.
⚪ Code Quality Violation
Consider adding super() or this() to your constructor (...read more)
In Java, it is suggested to call super() in an extended class. This rule will report a violation if both a call to super() and an overloaded constructor are absent.
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; |
There was a problem hiding this comment.
⚪ Code Quality Violation
Suggested change
| if (this == o) return true; | |
| if (this == o) {return true}; |
single if statement should be wrapped in a brace (...read more)
Omitting braces {} is valid in multiple statements, such as, for loops, if statements, and while loops. However, enforcing the use of control braces throughout your codebase will make the code more consistent and can make it easier to add statements in the future.
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; |
There was a problem hiding this comment.
⚪ Code Quality Violation
Suggested change
| if (o == null || getClass() != o.getClass()) return false; | |
| if (o == null || getClass() != o.getClass()) {return false}; |
single if statement should be wrapped in a brace (...read more)
Omitting braces {} is valid in multiple statements, such as, for loops, if statements, and while loops. However, enforcing the use of control braces throughout your codebase will make the code more consistent and can make it easier to add statements in the future.
nayeem-kamal
force-pushed
the
nayeem-kamal/spring-kafka-fix
branch
from
185ca1e
to
849d940
Compare
dougqh
reviewed
Oct 16, 2024
...n/java/datadog/trace/instrumentation/kafka_clients38/ConsumerCoordinatorInstrumentation.java
Outdated
Show resolved
Hide resolved
dougqh
approved these changes
Oct 16, 2024
amarziali
reviewed
Oct 17, 2024
|
|
||
| addTestSuite('latestDepTest') | ||
|
|
||
| //java { |
There was a problem hiding this comment.
Those comments should be cleaned out if not useful
There was a problem hiding this comment.
Good call. thanks! will address this in a separate PR.
amarziali
reviewed
Oct 17, 2024
| testImplementation group: 'javax.xml.bind', name: 'jaxb-api', version: '2.2.3' | ||
| testImplementation group: 'org.assertj', name: 'assertj-core', version: '2.9.+' | ||
| testImplementation group: 'org.mockito', name: 'mockito-core', version: '2.19.0' | ||
| testRuntimeOnly project(':dd-java-agent:instrumentation:spring-scheduling-3.1') |
There was a problem hiding this comment.
why do you need this dependency?
There was a problem hiding this comment.
This was carried over from the v0.11 instrumentation. I'll examine whether we need to keep this or not.
amarziali
reviewed
Oct 17, 2024
| [value, expected1, expected2, expected3, expected4]<< dataTable() | ||
| } | ||
|
|
||
| @Flaky |
There was a problem hiding this comment.
why are you adding a test if you know that's flaky?
There was a problem hiding this comment.
The reasoning for this is included in my PR description. I faced issues with getting testcontainers running in circleCI. The tests are passing locally but for some unknown reason that neither I nor the platform team has been able to resolve, we can't get it working in CI at the moment. Due to the demand from customers for this, Rohit has approved merging this with the flaky tests so long as the CI errors are resolved in the near future.
amarziali
reviewed
Oct 17, 2024
|
|
||
| @Override | ||
| boolean useStrictTraceWrites() { | ||
| // TODO fix this by making sure that spans get closed properly |
There was a problem hiding this comment.
have you tried to disable and see what's going wrong?
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What Does This Do
Adds support for APM and DSM for Kafka-client 3.8+. Included changes are fixes for supporting the new async consumer model as well as new instrumentation to collect Kafka cluster ID's and consumer groups
Adds a new configuration (
dd.trace.experimental.kafka.enabled) which must be set totruein order to enable this instrumentation.Motivation
Supporting DSM and APM customers that have missing traces and DSM data after upgrading to kafka 3.8
Additional Notes
At the time of merging, I faced issues with getting testcontainers running in circleCI. The tests written are all passing locally however they are marked as flaky since circleCI can't run them properly.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any usefull labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]