-
Notifications
You must be signed in to change notification settings - Fork 308
DSM optimizations - major refactoring to get rid of LinkedHashMap #9151
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
base: master
Are you sure you want to change the base?
Conversation
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 42 metrics, 11 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.52.0-SNAPSHOT~47b6142f95, baseline=1.52.0-SNAPSHOT~562e53388e
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (995.015 ms) : 0, 995015
Total [baseline] (10.609 s) : 0, 10609332
Agent [candidate] (996.85 ms) : 0, 996850
Total [candidate] (10.665 s) : 0, 10664710
section appsec
Agent [baseline] (1.179 s) : 0, 1178843
Total [baseline] (10.776 s) : 0, 10775554
Agent [candidate] (1.177 s) : 0, 1177083
Total [candidate] (10.762 s) : 0, 10762237
section iast
Agent [baseline] (1.129 s) : 0, 1129038
Total [baseline] (10.824 s) : 0, 10823702
Agent [candidate] (1.129 s) : 0, 1129323
Total [candidate] (10.783 s) : 0, 10783351
section profiling
Agent [baseline] (1.242 s) : 0, 1241514
Total [baseline] (11.048 s) : 0, 11048021
Agent [candidate] (1.244 s) : 0, 1243554
Total [candidate] (10.997 s) : 0, 10997233
gantt
title petclinic - break down per module: candidate=1.52.0-SNAPSHOT~47b6142f95, baseline=1.52.0-SNAPSHOT~562e53388e
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (686.409 ms) : 0, 686409
BytebuddyAgent [candidate] (687.548 ms) : 0, 687548
GlobalTracer [baseline] (242.397 ms) : 0, 242397
GlobalTracer [candidate] (242.896 ms) : 0, 242896
AppSec [baseline] (30.362 ms) : 0, 30362
AppSec [candidate] (30.502 ms) : 0, 30502
Debugger [baseline] (5.975 ms) : 0, 5975
Debugger [candidate] (6.068 ms) : 0, 6068
Remote Config [baseline] (673.195 µs) : 0, 673
Remote Config [candidate] (685.373 µs) : 0, 685
Telemetry [baseline] (8.253 ms) : 0, 8253
Telemetry [candidate] (8.253 ms) : 0, 8253
section appsec
BytebuddyAgent [baseline] (711.964 ms) : 0, 711964
BytebuddyAgent [candidate] (711.35 ms) : 0, 711350
GlobalTracer [baseline] (235.945 ms) : 0, 235945
GlobalTracer [candidate] (235.781 ms) : 0, 235781
AppSec [baseline] (171.974 ms) : 0, 171974
AppSec [candidate] (170.955 ms) : 0, 170955
Debugger [baseline] (5.778 ms) : 0, 5778
Debugger [candidate] (5.736 ms) : 0, 5736
Remote Config [baseline] (607.839 µs) : 0, 608
Remote Config [candidate] (602.626 µs) : 0, 603
Telemetry [baseline] (8.063 ms) : 0, 8063
Telemetry [candidate] (8.034 ms) : 0, 8034
IAST [baseline] (23.573 ms) : 0, 23573
IAST [candidate] (23.702 ms) : 0, 23702
section iast
BytebuddyAgent [baseline] (804.377 ms) : 0, 804377
BytebuddyAgent [candidate] (804.002 ms) : 0, 804002
GlobalTracer [baseline] (232.039 ms) : 0, 232039
GlobalTracer [candidate] (232.686 ms) : 0, 232686
AppSec [baseline] (31.135 ms) : 0, 31135
AppSec [candidate] (30.409 ms) : 0, 30409
Debugger [baseline] (6.656 ms) : 0, 6656
Debugger [candidate] (6.581 ms) : 0, 6581
Remote Config [baseline] (576.643 µs) : 0, 577
Remote Config [candidate] (587.16 µs) : 0, 587
Telemetry [baseline] (7.876 ms) : 0, 7876
Telemetry [candidate] (7.875 ms) : 0, 7875
IAST [baseline] (25.631 ms) : 0, 25631
IAST [candidate] (26.42 ms) : 0, 26420
section profiling
BytebuddyAgent [baseline] (675.953 ms) : 0, 675953
BytebuddyAgent [candidate] (676.843 ms) : 0, 676843
GlobalTracer [baseline] (361.181 ms) : 0, 361181
GlobalTracer [candidate] (361.688 ms) : 0, 361688
AppSec [baseline] (33.111 ms) : 0, 33111
AppSec [candidate] (32.48 ms) : 0, 32480
Debugger [baseline] (9.128 ms) : 0, 9128
Debugger [candidate] (11.265 ms) : 0, 11265
Remote Config [baseline] (655.075 µs) : 0, 655
Remote Config [candidate] (669.58 µs) : 0, 670
Telemetry [baseline] (9.469 ms) : 0, 9469
Telemetry [candidate] (8.697 ms) : 0, 8697
ProfilingAgent [baseline] (102.746 ms) : 0, 102746
ProfilingAgent [candidate] (103.21 ms) : 0, 103210
Profiling [baseline] (102.771 ms) : 0, 102771
Profiling [candidate] (103.234 ms) : 0, 103234
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.52.0-SNAPSHOT~47b6142f95, baseline=1.52.0-SNAPSHOT~562e53388e
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.011 s) : 0, 1010532
Total [baseline] (8.575 s) : 0, 8575015
Agent [candidate] (994.973 ms) : 0, 994973
Total [candidate] (8.565 s) : 0, 8565277
section iast
Agent [baseline] (1.128 s) : 0, 1127716
Total [baseline] (9.305 s) : 0, 9305301
Agent [candidate] (1.13 s) : 0, 1130058
Total [candidate] (9.327 s) : 0, 9326868
gantt
title insecure-bank - break down per module: candidate=1.52.0-SNAPSHOT~47b6142f95, baseline=1.52.0-SNAPSHOT~562e53388e
dateFormat X
axisFormat %s
section tracing
BytebuddyAgent [baseline] (699.096 ms) : 0, 699096
BytebuddyAgent [candidate] (686.619 ms) : 0, 686619
GlobalTracer [baseline] (244.544 ms) : 0, 244544
GlobalTracer [candidate] (242.357 ms) : 0, 242357
AppSec [baseline] (30.722 ms) : 0, 30722
AppSec [candidate] (30.4 ms) : 0, 30400
Debugger [baseline] (6.013 ms) : 0, 6013
Debugger [candidate] (5.995 ms) : 0, 5995
Remote Config [baseline] (698.375 µs) : 0, 698
Remote Config [candidate] (685.15 µs) : 0, 685
Telemetry [baseline] (8.392 ms) : 0, 8392
Telemetry [candidate] (8.204 ms) : 0, 8204
section iast
BytebuddyAgent [baseline] (803.35 ms) : 0, 803350
BytebuddyAgent [candidate] (805.055 ms) : 0, 805055
GlobalTracer [baseline] (231.652 ms) : 0, 231652
GlobalTracer [candidate] (232.759 ms) : 0, 232759
AppSec [baseline] (28.168 ms) : 0, 28168
AppSec [candidate] (29.416 ms) : 0, 29416
Debugger [baseline] (6.526 ms) : 0, 6526
Debugger [candidate] (6.578 ms) : 0, 6578
Remote Config [baseline] (576.812 µs) : 0, 577
Remote Config [candidate] (575.23 µs) : 0, 575
Telemetry [baseline] (7.905 ms) : 0, 7905
Telemetry [candidate] (7.91 ms) : 0, 7910
IAST [baseline] (28.886 ms) : 0, 28886
IAST [candidate] (27.128 ms) : 0, 27128
LoadParameters
See matching parameters
SummaryFound 2 performance improvements and 3 performance regressions! Performance is the same for 7 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~47b6142f95, baseline=1.52.0-SNAPSHOT~562e53388e
dateFormat X
axisFormat %s
section baseline
no_agent (37.098 ms) : 36799, 37397
. : milestone, 37098,
appsec (46.546 ms) : 46112, 46980
. : milestone, 46546,
code_origins (43.925 ms) : 43548, 44301
. : milestone, 43925,
iast (43.905 ms) : 43513, 44298
. : milestone, 43905,
profiling (49.853 ms) : 49401, 50306
. : milestone, 49853,
tracing (43.615 ms) : 43261, 43969
. : milestone, 43615,
section candidate
no_agent (36.235 ms) : 35945, 36525
. : milestone, 36235,
appsec (49.623 ms) : 49174, 50072
. : milestone, 49623,
code_origins (44.648 ms) : 44262, 45034
. : milestone, 44648,
iast (45.665 ms) : 45262, 46069
. : milestone, 45665,
profiling (48.159 ms) : 47696, 48621
. : milestone, 48159,
tracing (44.294 ms) : 43929, 44659
. : milestone, 44294,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.52.0-SNAPSHOT~47b6142f95, baseline=1.52.0-SNAPSHOT~562e53388e
dateFormat X
axisFormat %s
section baseline
no_agent (4.214 ms) : 4163, 4265
. : milestone, 4214,
iast (9.14 ms) : 8991, 9289
. : milestone, 9140,
iast_FULL (13.644 ms) : 13374, 13914
. : milestone, 13644,
iast_GLOBAL (10.379 ms) : 10194, 10565
. : milestone, 10379,
profiling (8.585 ms) : 8453, 8717
. : milestone, 8585,
tracing (7.817 ms) : 7706, 7927
. : milestone, 7817,
section candidate
no_agent (4.357 ms) : 4305, 4409
. : milestone, 4357,
iast (9.184 ms) : 9032, 9335
. : milestone, 9184,
iast_FULL (13.792 ms) : 13519, 14065
. : milestone, 13792,
iast_GLOBAL (9.927 ms) : 9755, 10099
. : milestone, 9927,
profiling (8.679 ms) : 8543, 8815
. : milestone, 8679,
tracing (7.647 ms) : 7537, 7757
. : milestone, 7647,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 12 metrics, 0 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~47b6142f95, baseline=1.52.0-SNAPSHOT~562e53388e
dateFormat X
axisFormat %s
section baseline
no_agent (1.471 ms) : 1459, 1482
. : milestone, 1471,
appsec (2.417 ms) : 2366, 2467
. : milestone, 2417,
iast (2.193 ms) : 2131, 2256
. : milestone, 2193,
iast_GLOBAL (2.251 ms) : 2187, 2315
. : milestone, 2251,
profiling (2.057 ms) : 2005, 2109
. : milestone, 2057,
tracing (2.023 ms) : 1974, 2072
. : milestone, 2023,
section candidate
no_agent (1.474 ms) : 1462, 1485
. : milestone, 1474,
appsec (2.411 ms) : 2361, 2461
. : milestone, 2411,
iast (2.191 ms) : 2128, 2254
. : milestone, 2191,
iast_GLOBAL (2.236 ms) : 2173, 2300
. : milestone, 2236,
profiling (2.068 ms) : 2015, 2121
. : milestone, 2068,
tracing (2.009 ms) : 1960, 2058
. : milestone, 2009,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.52.0-SNAPSHOT~47b6142f95, baseline=1.52.0-SNAPSHOT~562e53388e
dateFormat X
axisFormat %s
section baseline
no_agent (14.95 s) : 14950000, 14950000
. : milestone, 14950000,
appsec (14.774 s) : 14774000, 14774000
. : milestone, 14774000,
iast (18.553 s) : 18553000, 18553000
. : milestone, 18553000,
iast_GLOBAL (18.026 s) : 18026000, 18026000
. : milestone, 18026000,
profiling (15.585 s) : 15585000, 15585000
. : milestone, 15585000,
tracing (14.931 s) : 14931000, 14931000
. : milestone, 14931000,
section candidate
no_agent (15.493 s) : 15493000, 15493000
. : milestone, 15493000,
appsec (14.7 s) : 14700000, 14700000
. : milestone, 14700000,
iast (18.531 s) : 18531000, 18531000
. : milestone, 18531000,
iast_GLOBAL (17.952 s) : 17952000, 17952000
. : milestone, 17952000,
profiling (15.23 s) : 15230000, 15230000
. : milestone, 15230000,
tracing (14.995 s) : 14995000, 14995000
. : milestone, 14995000,
|
Kafka / producer-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
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 1 performance regressions! Performance is the same for 2 metrics, 0 unstable metrics.
See unchanged results
|
result.put(DIRECTION_TAG, DIRECTION_OUT); | ||
result.put(TYPE_TAG, "grpc"); | ||
return DataStreamsContext.fromTags(result); | ||
return DataStreamsContext.fromTags( |
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.
I would avoid builder idioms in the critical path. That's still extra allocation that we really don't need.
|
||
public DataStreamsTags(DataStreamsTagsBuilder builder) { | ||
this.builder = builder; | ||
this.size = |
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.
This is introducing a capturing lambda. I think a regular loop would probably be better here.
processor.process(BUS_TAG, this.builder.bus); | ||
count += 1; | ||
} | ||
|
||
if (this.builder.direction == Direction.Inbound) { | ||
count += 1; | ||
processor.process(DIRECTION_TAG, DIRECTION_IN); | ||
} else if (this.builder.direction == Direction.Outbound) { | ||
count += 1; | ||
processor.process(DIRECTION_TAG, DIRECTION_OUT); |
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.
Suggestion: Just a matter of taste, but probably count++;
Would be a better option (here and other similar places)?
+ "," | ||
+ ", kafkaClusterId='" | ||
+ this.builder.kafkaClusterId | ||
+ "," | ||
+ ", partition='" | ||
+ this.builder.partition | ||
+ "," | ||
+ ", hash=" | ||
+ hash | ||
+ "," | ||
+ ", aggregationHash=" | ||
+ aggregationHash | ||
+ "," | ||
+ ", size=" |
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.
I think this code will produce String
with TWO commas. Is that expected?
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.
Yes, you are right... AI autosuggest :D
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.
Nice :), just FYI IDEA has toString()
method generation too,
result.put(TYPE_TAG, "grpc"); | ||
return result; | ||
private static final DataStreamsTags createServerPathwaySortedTags() { | ||
return new DataStreamsTagsBuilder() |
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.
I think we can probably still keep these pretty clean without needing a builder. I've put suggestions throughout...
- createGrpcInbound() <-- bonus, that's cacheable
DataStreamsContext dsmContext = DataStreamsContext.fromTags(getTags(eventBusName)); | ||
DataStreamsTags tags = | ||
new DataStreamsTagsBuilder() | ||
.withDirection(DataStreamsTags.Direction.Outbound) |
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.
createBusOutbound(eventBusName)
sortedTags.put(TagsProcessor.TOPIC_TAG, bucket); | ||
sortedTags.put(TagsProcessor.TYPE_TAG, "s3"); | ||
DataStreamsTags tags = | ||
new DataStreamsTagsBuilder() |
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.
createS3Outbound(bucket, key)
sortedTags.put(TYPE_TAG, "kinesis"); | ||
DataStreamsTags tags = | ||
new DataStreamsTagsBuilder() | ||
.withType("kinesis") |
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.
createKinesisInbound(streamArn)
|
||
return sortedTags; | ||
private DataStreamsTags getTags(String snsTopicName) { | ||
return new DataStreamsTagsBuilder() |
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.
createSnsOutbound(snsTopicName)
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 you're in refactoring mode, can you have a look (made in another PR if it's easier) to complete the context refactoring?
In short, PathwayContext
should not be stored in span data model (DDSpanContext
).
Instead, it must be stored with Context
as DataStreamsContext
and AgentSpan
.
You may create a second Context
entry if the DataStreamsContext
holds different kind of data (or have a different lifecycle) than what PathwayContext
should hold.
I already done some refactoring (adding DataStreamsContext
, creating a dedicated DSM propagator) but it miss the last step of moving away PathwayContext
from span.
Would that be something you could have a look at? Would you like us to pair on this task? The end goal would be to decouple Tracing from DSM. So you can save even more resources.
@@ -89,7 +85,9 @@ private String getTraceContextToInject( | |||
// Inject context | |||
datadog.context.Context context = span; | |||
if (traceConfig().isDataStreamsEnabled()) { | |||
DataStreamsContext dsmContext = DataStreamsContext.fromTags(getTags(eventBusName)); | |||
DataStreamsTags tags = | |||
DataStreamsTags.createWithBus("bus", DataStreamsTags.Direction.Outbound, eventBusName); |
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.
Why pass "bus" as argument to withBus helper? Is there a case where the value isn't "bus"?
private long completeHash; | ||
private int nonNullSize; | ||
|
||
// hash tags |
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.
These should probably be final
Right now, strictly speaking this probably isn't thread safe
kafkaClusterId != null ? KAFKA_CLUSTER_ID_TAG + ":" + kafkaClusterId : null; | ||
this.partition = partition != null ? PARTITION_TAG + ":" + partition : null; | ||
|
||
// hashable tags are 0-4 |
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.
Nice, that should unroll and inline nicely
This change is aimed to reduce DSM overhead by using a lightweight object instead of linked maps.
Introduced
DataStreamsTags
andDataStreamsTagsBuild
which are now used instead ofLinkedHashMap
.Updated all integrations and tests.
My tests show ~10% throughput increase with a significant latency decrease without increasing CPU load.
