-
Notifications
You must be signed in to change notification settings - Fork 324
Improve virtual thread context tracking support #10208
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
Conversation
9d87e39 to
bae768a
Compare
bric3
left a comment
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.
No blockers, but I believe a few points should be fixed.
...rumentation-testing/src/main/java/datadog/trace/agent/test/ClassFileTransformerListener.java
Show resolved
Hide resolved
...rumentation-testing/src/main/java/datadog/trace/agent/test/ClassFileTransformerListener.java
Show resolved
Hide resolved
...rumentation-testing/src/main/java/datadog/trace/agent/test/ClassFileTransformerListener.java
Show resolved
Hide resolved
...trumentation-testing/src/main/java/datadog/trace/agent/test/AbstractInstrumentationTest.java
Show resolved
Hide resolved
...trumentation-testing/src/main/java/datadog/trace/agent/test/AbstractInstrumentationTest.java
Show resolved
Hide resolved
| long deadline = System.currentTimeMillis() + TIMEOUT_MILLIS; | ||
|
|
||
| while (pendingTrace.size() < numberOfSpans) { | ||
| if (System.currentTimeMillis() > deadline) { |
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: You're probably better off with nanos, as currentTimeMillis can change for various reasons like NTP adjustments, even for tests I think it's preferable to use nanos.
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 a port from the Groovy implementation:
Line 611 in c426f8c
| final long deadline = System.currentTimeMillis() + TIMEOUT_MILLIS |
I would recommend using a dedicated PR to solve it separately because having different implementation for the same logic will bite us later 😅 But yes, this is not the right way, even if nanos is more expensive, we don't care for testing.
...trumentation-testing/src/main/java/datadog/trace/agent/test/AbstractInstrumentationTest.java
Show resolved
Hide resolved
| /** Verifies the parent / child span relation. */ | ||
| void assertConnectedTrace() { |
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:
| /** Verifies the parent / child span relation. */ | |
| void assertConnectedTrace() { | |
| void assertParentChildRelationship() { |
| trace.sort(comparing(DDSpan::getStartTimeNano)); | ||
| assertEquals(4, trace.size()); | ||
| assertEquals("parent", trace.get(0).getOperationName()); | ||
| assertEquals("child", trace.get(1).getOperationName()); | ||
| assertEquals("great-child", trace.get(2).getOperationName()); | ||
| assertEquals("great-great-child", trace.get(3).getOperationName()); | ||
| assertEquals(trace.get(0).getSpanId(), trace.get(1).getParentId()); | ||
| assertEquals(trace.get(1).getSpanId(), trace.get(2).getParentId()); | ||
| assertEquals(trace.get(2).getSpanId(), trace.get(3).getParentId()); |
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.
thought: I wonder if it's possible to craft a "generic" method that can do that for any number of child.
It would be called
assertParentChildRelationship(Map.of(
"parent", "child",
"child", "great-child",
"great-child", "great-great-child"
))
...It might be outside the scope of this PR though.
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, it's possible and already implemented. I did it as part of previous R&D week. But that's way out of scope for this PR 😅
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 56 metrics, 9 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.57.0-SNAPSHOT~bae768a2d4, baseline=1.57.0-SNAPSHOT~552ae2be7d
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.08 s) : 0, 1080471
Total [baseline] (10.955 s) : 0, 10954647
Agent [candidate] (1.083 s) : 0, 1083131
Total [candidate] (10.889 s) : 0, 10888524
section appsec
Agent [baseline] (1.268 s) : 0, 1267688
Total [baseline] (11.004 s) : 0, 11004393
Agent [candidate] (1.272 s) : 0, 1272335
Total [candidate] (11.063 s) : 0, 11062595
section iast
Agent [baseline] (1.232 s) : 0, 1232183
Total [baseline] (11.285 s) : 0, 11284761
Agent [candidate] (1.223 s) : 0, 1222704
Total [candidate] (11.253 s) : 0, 11253499
section profiling
Agent [baseline] (1.208 s) : 0, 1207868
Total [baseline] (11.0 s) : 0, 10999929
Agent [candidate] (1.213 s) : 0, 1213276
Total [candidate] (10.957 s) : 0, 10956881
gantt
title petclinic - break down per module: candidate=1.57.0-SNAPSHOT~bae768a2d4, baseline=1.57.0-SNAPSHOT~552ae2be7d
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.183 ms) : 0, 1183
crashtracking [candidate] (1.201 ms) : 0, 1201
BytebuddyAgent [baseline] (648.618 ms) : 0, 648618
BytebuddyAgent [candidate] (650.024 ms) : 0, 650024
GlobalTracer [baseline] (281.566 ms) : 0, 281566
GlobalTracer [candidate] (282.343 ms) : 0, 282343
AppSec [baseline] (32.296 ms) : 0, 32296
AppSec [candidate] (32.52 ms) : 0, 32520
Debugger [baseline] (67.773 ms) : 0, 67773
Debugger [candidate] (67.962 ms) : 0, 67962
Remote Config [baseline] (655.121 µs) : 0, 655
Remote Config [candidate] (624.368 µs) : 0, 624
Telemetry [baseline] (9.045 ms) : 0, 9045
Telemetry [candidate] (9.088 ms) : 0, 9088
Flare Poller [baseline] (3.725 ms) : 0, 3725
Flare Poller [candidate] (3.81 ms) : 0, 3810
section appsec
crashtracking [baseline] (1.191 ms) : 0, 1191
crashtracking [candidate] (1.213 ms) : 0, 1213
BytebuddyAgent [baseline] (690.385 ms) : 0, 690385
BytebuddyAgent [candidate] (693.713 ms) : 0, 693713
GlobalTracer [baseline] (259.364 ms) : 0, 259364
GlobalTracer [candidate] (259.96 ms) : 0, 259960
IAST [baseline] (24.607 ms) : 0, 24607
IAST [candidate] (24.734 ms) : 0, 24734
AppSec [baseline] (175.206 ms) : 0, 175206
AppSec [candidate] (175.677 ms) : 0, 175677
Debugger [baseline] (67.304 ms) : 0, 67304
Debugger [candidate] (67.633 ms) : 0, 67633
Remote Config [baseline] (724.809 µs) : 0, 725
Remote Config [candidate] (760.595 µs) : 0, 761
Telemetry [baseline] (9.154 ms) : 0, 9154
Telemetry [candidate] (9.122 ms) : 0, 9122
Flare Poller [baseline] (4.014 ms) : 0, 4014
Flare Poller [candidate] (3.853 ms) : 0, 3853
section iast
crashtracking [baseline] (1.196 ms) : 0, 1196
crashtracking [candidate] (1.189 ms) : 0, 1189
BytebuddyAgent [baseline] (797.426 ms) : 0, 797426
BytebuddyAgent [candidate] (790.283 ms) : 0, 790283
GlobalTracer [baseline] (256.986 ms) : 0, 256986
GlobalTracer [candidate] (255.916 ms) : 0, 255916
IAST [baseline] (27.167 ms) : 0, 27167
IAST [candidate] (26.929 ms) : 0, 26929
AppSec [baseline] (35.308 ms) : 0, 35308
AppSec [candidate] (35.377 ms) : 0, 35377
Debugger [baseline] (65.882 ms) : 0, 65882
Debugger [candidate] (65.062 ms) : 0, 65062
Remote Config [baseline] (545.418 µs) : 0, 545
Remote Config [candidate] (568.209 µs) : 0, 568
Telemetry [baseline] (8.521 ms) : 0, 8521
Telemetry [candidate] (8.449 ms) : 0, 8449
Flare Poller [baseline] (3.536 ms) : 0, 3536
Flare Poller [candidate] (3.51 ms) : 0, 3510
section profiling
crashtracking [baseline] (1.181 ms) : 0, 1181
crashtracking [candidate] (1.194 ms) : 0, 1194
BytebuddyAgent [baseline] (703.436 ms) : 0, 703436
BytebuddyAgent [candidate] (708.009 ms) : 0, 708009
GlobalTracer [baseline] (221.267 ms) : 0, 221267
GlobalTracer [candidate] (222.637 ms) : 0, 222637
AppSec [baseline] (32.149 ms) : 0, 32149
AppSec [candidate] (32.476 ms) : 0, 32476
Debugger [baseline] (68.496 ms) : 0, 68496
Debugger [candidate] (68.477 ms) : 0, 68477
Remote Config [baseline] (621.221 µs) : 0, 621
Remote Config [candidate] (627.05 µs) : 0, 627
Telemetry [baseline] (9.009 ms) : 0, 9009
Telemetry [candidate] (9.149 ms) : 0, 9149
Flare Poller [baseline] (3.817 ms) : 0, 3817
Flare Poller [candidate] (3.785 ms) : 0, 3785
ProfilingAgent [baseline] (97.825 ms) : 0, 97825
ProfilingAgent [candidate] (96.549 ms) : 0, 96549
Profiling [baseline] (98.405 ms) : 0, 98405
Profiling [candidate] (97.113 ms) : 0, 97113
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.57.0-SNAPSHOT~bae768a2d4, baseline=1.57.0-SNAPSHOT~552ae2be7d
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.086 s) : 0, 1086477
Total [baseline] (8.797 s) : 0, 8796798
Agent [candidate] (1.088 s) : 0, 1088441
Total [candidate] (8.785 s) : 0, 8784724
section iast
Agent [baseline] (1.221 s) : 0, 1220701
Total [baseline] (9.401 s) : 0, 9400891
Agent [candidate] (1.233 s) : 0, 1232826
Total [candidate] (9.446 s) : 0, 9445873
gantt
title insecure-bank - break down per module: candidate=1.57.0-SNAPSHOT~bae768a2d4, baseline=1.57.0-SNAPSHOT~552ae2be7d
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.191 ms) : 0, 1191
crashtracking [candidate] (1.199 ms) : 0, 1199
BytebuddyAgent [baseline] (651.617 ms) : 0, 651617
BytebuddyAgent [candidate] (654.362 ms) : 0, 654362
GlobalTracer [baseline] (284.203 ms) : 0, 284203
GlobalTracer [candidate] (283.907 ms) : 0, 283907
AppSec [baseline] (32.65 ms) : 0, 32650
AppSec [candidate] (32.571 ms) : 0, 32571
Debugger [baseline] (67.547 ms) : 0, 67547
Debugger [candidate] (67.304 ms) : 0, 67304
Remote Config [baseline] (667.508 µs) : 0, 668
Remote Config [candidate] (642.471 µs) : 0, 642
Telemetry [baseline] (9.049 ms) : 0, 9049
Telemetry [candidate] (8.938 ms) : 0, 8938
Flare Poller [baseline] (3.849 ms) : 0, 3849
Flare Poller [candidate] (3.735 ms) : 0, 3735
section iast
crashtracking [baseline] (1.204 ms) : 0, 1204
crashtracking [candidate] (1.196 ms) : 0, 1196
BytebuddyAgent [baseline] (789.529 ms) : 0, 789529
BytebuddyAgent [candidate] (798.92 ms) : 0, 798920
GlobalTracer [baseline] (255.642 ms) : 0, 255642
GlobalTracer [candidate] (257.657 ms) : 0, 257657
IAST [baseline] (26.835 ms) : 0, 26835
IAST [candidate] (27.209 ms) : 0, 27209
AppSec [baseline] (33.807 ms) : 0, 33807
AppSec [candidate] (32.133 ms) : 0, 32133
Debugger [baseline] (65.723 ms) : 0, 65723
Debugger [candidate] (67.563 ms) : 0, 67563
Remote Config [baseline] (583.227 µs) : 0, 583
Remote Config [candidate] (548.773 µs) : 0, 549
Telemetry [baseline] (8.38 ms) : 0, 8380
Telemetry [candidate] (8.42 ms) : 0, 8420
Flare Poller [baseline] (3.474 ms) : 0, 3474
Flare Poller [candidate] (3.502 ms) : 0, 3502
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 1 performance regressions! Performance is the same for 19 metrics, 15 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.57.0-SNAPSHOT~bae768a2d4, baseline=1.57.0-SNAPSHOT~552ae2be7d
dateFormat X
axisFormat %s
section baseline
no_agent (20.159 ms) : 19951, 20367
. : milestone, 20159,
appsec (18.785 ms) : 18592, 18978
. : milestone, 18785,
code_origins (17.821 ms) : 17644, 17998
. : milestone, 17821,
iast (17.802 ms) : 17624, 17980
. : milestone, 17802,
profiling (18.647 ms) : 18460, 18835
. : milestone, 18647,
tracing (17.78 ms) : 17602, 17958
. : milestone, 17780,
section candidate
no_agent (19.358 ms) : 19159, 19557
. : milestone, 19358,
appsec (19.488 ms) : 19289, 19688
. : milestone, 19488,
code_origins (17.807 ms) : 17626, 17987
. : milestone, 17807,
iast (17.707 ms) : 17529, 17885
. : milestone, 17707,
profiling (18.658 ms) : 18474, 18842
. : milestone, 18658,
tracing (17.84 ms) : 17657, 18022
. : milestone, 17840,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.57.0-SNAPSHOT~bae768a2d4, baseline=1.57.0-SNAPSHOT~552ae2be7d
dateFormat X
axisFormat %s
section baseline
no_agent (1.202 ms) : 1190, 1214
. : milestone, 1202,
iast (3.118 ms) : 3079, 3157
. : milestone, 3118,
iast_FULL (5.728 ms) : 5671, 5785
. : milestone, 5728,
iast_GLOBAL (3.626 ms) : 3564, 3688
. : milestone, 3626,
profiling (2.235 ms) : 2215, 2255
. : milestone, 2235,
tracing (1.811 ms) : 1795, 1826
. : milestone, 1811,
section candidate
no_agent (1.199 ms) : 1189, 1209
. : milestone, 1199,
iast (3.092 ms) : 3053, 3131
. : milestone, 3092,
iast_FULL (5.627 ms) : 5571, 5682
. : milestone, 5627,
iast_GLOBAL (3.427 ms) : 3379, 3475
. : milestone, 3427,
profiling (2.218 ms) : 2199, 2237
. : milestone, 2218,
tracing (1.821 ms) : 1806, 1837
. : milestone, 1821,
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.57.0-SNAPSHOT~bae768a2d4, baseline=1.57.0-SNAPSHOT~552ae2be7d
dateFormat X
axisFormat %s
section baseline
no_agent (1.478 ms) : 1467, 1490
. : milestone, 1478,
appsec (2.48 ms) : 2428, 2533
. : milestone, 2480,
iast (2.219 ms) : 2154, 2283
. : milestone, 2219,
iast_GLOBAL (2.269 ms) : 2204, 2334
. : milestone, 2269,
profiling (2.083 ms) : 2030, 2135
. : milestone, 2083,
tracing (2.054 ms) : 2003, 2105
. : milestone, 2054,
section candidate
no_agent (1.486 ms) : 1475, 1498
. : milestone, 1486,
appsec (2.472 ms) : 2420, 2524
. : milestone, 2472,
iast (2.236 ms) : 2171, 2301
. : milestone, 2236,
iast_GLOBAL (2.266 ms) : 2201, 2331
. : milestone, 2266,
profiling (2.098 ms) : 2043, 2152
. : milestone, 2098,
tracing (2.059 ms) : 2008, 2110
. : milestone, 2059,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.57.0-SNAPSHOT~bae768a2d4, baseline=1.57.0-SNAPSHOT~552ae2be7d
dateFormat X
axisFormat %s
section baseline
no_agent (14.889 s) : 14889000, 14889000
. : milestone, 14889000,
appsec (14.516 s) : 14516000, 14516000
. : milestone, 14516000,
iast (18.1 s) : 18100000, 18100000
. : milestone, 18100000,
iast_GLOBAL (17.679 s) : 17679000, 17679000
. : milestone, 17679000,
profiling (14.708 s) : 14708000, 14708000
. : milestone, 14708000,
tracing (14.751 s) : 14751000, 14751000
. : milestone, 14751000,
section candidate
no_agent (15.663 s) : 15663000, 15663000
. : milestone, 15663000,
appsec (14.84 s) : 14840000, 14840000
. : milestone, 14840000,
iast (18.248 s) : 18248000, 18248000
. : milestone, 18248000,
iast_GLOBAL (17.876 s) : 17876000, 17876000
. : milestone, 17876000,
profiling (15.167 s) : 15167000, 15167000
. : milestone, 15167000,
tracing (14.558 s) : 14558000, 14558000
. : milestone, 14558000,
|
bae768a to
16c03da
Compare
What Does This Do
This PR introduces a minimal instrumentation unit tests capability (trace asserts are in another PRs) and migrates the Virtual Thread context tracking support to it.
Motivation
Additional Notes
Use
testdog.trace.as root package for the tests asdatadog.trace.is excluded from instrumented packages.Contributor Checklist
type:and (comp:orinst:) labels in addition to any useful labelsclose,fixor any linking keywords when referencing an issue.Use
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]