Conversation
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
Debugger benchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 6 unstable metrics. See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (323.054 µs) : 273, 373
. : milestone, 323,
basic (279.718 µs) : 273, 286
. : milestone, 280,
loop (8.969 ms) : 8965, 8974
. : milestone, 8969,
section candidate
noprobe (315.722 µs) : 283, 349
. : milestone, 316,
basic (274.309 µs) : 268, 281
. : milestone, 274,
loop (8.952 ms) : 8948, 8956
. : milestone, 8952,
|
BenchmarksStartupParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 60 metrics, 11 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.60.0-SNAPSHOT~5ba371eb8e, baseline=1.60.0-SNAPSHOT~af8b84438c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.064 s) : 0, 1063929
Total [baseline] (10.887 s) : 0, 10886846
Agent [candidate] (1.08 s) : 0, 1080421
Total [candidate] (10.907 s) : 0, 10906635
section appsec
Agent [baseline] (1.241 s) : 0, 1240556
Total [baseline] (11.038 s) : 0, 11038171
Agent [candidate] (1.24 s) : 0, 1240035
Total [candidate] (11.078 s) : 0, 11077503
section iast
Agent [baseline] (1.244 s) : 0, 1244376
Total [baseline] (11.292 s) : 0, 11291646
Agent [candidate] (1.233 s) : 0, 1233389
Total [candidate] (11.26 s) : 0, 11259674
section profiling
Agent [baseline] (1.191 s) : 0, 1190895
Total [baseline] (10.891 s) : 0, 10890710
Agent [candidate] (1.193 s) : 0, 1192607
Total [candidate] (10.965 s) : 0, 10964843
gantt
title petclinic - break down per module: candidate=1.60.0-SNAPSHOT~5ba371eb8e, baseline=1.60.0-SNAPSHOT~af8b84438c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.192 ms) : 0, 1192
crashtracking [candidate] (1.206 ms) : 0, 1206
BytebuddyAgent [baseline] (627.287 ms) : 0, 627287
BytebuddyAgent [candidate] (636.904 ms) : 0, 636904
AgentMeter [baseline] (29.059 ms) : 0, 29059
AgentMeter [candidate] (29.513 ms) : 0, 29513
GlobalTracer [baseline] (257.29 ms) : 0, 257290
GlobalTracer [candidate] (260.515 ms) : 0, 260515
AppSec [baseline] (32.986 ms) : 0, 32986
AppSec [candidate] (33.63 ms) : 0, 33630
Debugger [baseline] (64.955 ms) : 0, 64955
Debugger [candidate] (66.297 ms) : 0, 66297
Remote Config [baseline] (632.392 µs) : 0, 632
Remote Config [candidate] (623.461 µs) : 0, 623
Telemetry [baseline] (9.909 ms) : 0, 9909
Telemetry [candidate] (9.206 ms) : 0, 9206
Flare Poller [baseline] (4.573 ms) : 0, 4573
Flare Poller [candidate] (6.215 ms) : 0, 6215
section appsec
crashtracking [baseline] (1.184 ms) : 0, 1184
crashtracking [candidate] (1.193 ms) : 0, 1193
BytebuddyAgent [baseline] (659.213 ms) : 0, 659213
BytebuddyAgent [candidate] (658.679 ms) : 0, 658679
AgentMeter [baseline] (12.025 ms) : 0, 12025
AgentMeter [candidate] (12.006 ms) : 0, 12006
GlobalTracer [baseline] (258.97 ms) : 0, 258970
GlobalTracer [candidate] (258.374 ms) : 0, 258374
IAST [baseline] (25.481 ms) : 0, 25481
IAST [candidate] (25.368 ms) : 0, 25368
AppSec [baseline] (168.774 ms) : 0, 168774
AppSec [candidate] (167.864 ms) : 0, 167864
Debugger [baseline] (65.341 ms) : 0, 65341
Debugger [candidate] (66.663 ms) : 0, 66663
Remote Config [baseline] (626.383 µs) : 0, 626
Remote Config [candidate] (639.257 µs) : 0, 639
Telemetry [baseline] (9.19 ms) : 0, 9190
Telemetry [candidate] (9.384 ms) : 0, 9384
Flare Poller [baseline] (3.634 ms) : 0, 3634
Flare Poller [candidate] (3.737 ms) : 0, 3737
section iast
crashtracking [baseline] (1.212 ms) : 0, 1212
crashtracking [candidate] (1.193 ms) : 0, 1193
BytebuddyAgent [baseline] (804.153 ms) : 0, 804153
BytebuddyAgent [candidate] (796.206 ms) : 0, 796206
AgentMeter [baseline] (11.617 ms) : 0, 11617
AgentMeter [candidate] (11.316 ms) : 0, 11316
GlobalTracer [baseline] (249.771 ms) : 0, 249771
GlobalTracer [candidate] (248.171 ms) : 0, 248171
IAST [baseline] (27.337 ms) : 0, 27337
IAST [candidate] (27.01 ms) : 0, 27010
AppSec [baseline] (35.247 ms) : 0, 35247
AppSec [candidate] (34.939 ms) : 0, 34939
Debugger [baseline] (66.099 ms) : 0, 66099
Debugger [candidate] (65.911 ms) : 0, 65911
Remote Config [baseline] (534.5 µs) : 0, 534
Remote Config [candidate] (541.121 µs) : 0, 541
Telemetry [baseline] (8.658 ms) : 0, 8658
Telemetry [candidate] (8.575 ms) : 0, 8575
Flare Poller [baseline] (3.468 ms) : 0, 3468
Flare Poller [candidate] (3.481 ms) : 0, 3481
section profiling
crashtracking [baseline] (1.178 ms) : 0, 1178
crashtracking [candidate] (1.184 ms) : 0, 1184
BytebuddyAgent [baseline] (680.681 ms) : 0, 680681
BytebuddyAgent [candidate] (682.711 ms) : 0, 682711
AgentMeter [baseline] (8.519 ms) : 0, 8519
AgentMeter [candidate] (8.546 ms) : 0, 8546
GlobalTracer [baseline] (215.736 ms) : 0, 215736
GlobalTracer [candidate] (216.023 ms) : 0, 216023
AppSec [baseline] (32.689 ms) : 0, 32689
AppSec [candidate] (32.559 ms) : 0, 32559
Debugger [baseline] (67.97 ms) : 0, 67970
Debugger [candidate] (67.465 ms) : 0, 67465
Remote Config [baseline] (644.845 µs) : 0, 645
Remote Config [candidate] (663.52 µs) : 0, 664
Telemetry [baseline] (9.046 ms) : 0, 9046
Telemetry [candidate] (9.057 ms) : 0, 9057
Flare Poller [baseline] (3.77 ms) : 0, 3770
Flare Poller [candidate] (3.739 ms) : 0, 3739
ProfilingAgent [baseline] (100.111 ms) : 0, 100111
ProfilingAgent [candidate] (99.798 ms) : 0, 99798
Profiling [baseline] (100.719 ms) : 0, 100719
Profiling [candidate] (100.368 ms) : 0, 100368
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.60.0-SNAPSHOT~5ba371eb8e, baseline=1.60.0-SNAPSHOT~af8b84438c
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.065 s) : 0, 1064735
Total [baseline] (8.766 s) : 0, 8766416
Agent [candidate] (1.067 s) : 0, 1066887
Total [candidate] (8.769 s) : 0, 8769485
section iast
Agent [baseline] (1.235 s) : 0, 1235016
Total [baseline] (9.37 s) : 0, 9369523
Agent [candidate] (1.232 s) : 0, 1232345
Total [candidate] (9.372 s) : 0, 9371843
gantt
title insecure-bank - break down per module: candidate=1.60.0-SNAPSHOT~5ba371eb8e, baseline=1.60.0-SNAPSHOT~af8b84438c
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.2 ms) : 0, 1200
crashtracking [candidate] (1.231 ms) : 0, 1231
BytebuddyAgent [baseline] (628.073 ms) : 0, 628073
BytebuddyAgent [candidate] (627.74 ms) : 0, 627740
AgentMeter [baseline] (29.142 ms) : 0, 29142
AgentMeter [candidate] (29.063 ms) : 0, 29063
GlobalTracer [baseline] (257.499 ms) : 0, 257499
GlobalTracer [candidate] (259.267 ms) : 0, 259267
AppSec [baseline] (32.997 ms) : 0, 32997
AppSec [candidate] (33.398 ms) : 0, 33398
Debugger [baseline] (65.336 ms) : 0, 65336
Debugger [candidate] (64.096 ms) : 0, 64096
Remote Config [baseline] (636.234 µs) : 0, 636
Remote Config [candidate] (611.931 µs) : 0, 612
Telemetry [baseline] (9.093 ms) : 0, 9093
Telemetry [candidate] (10.692 ms) : 0, 10692
Flare Poller [baseline] (4.514 ms) : 0, 4514
Flare Poller [candidate] (4.664 ms) : 0, 4664
section iast
crashtracking [baseline] (1.214 ms) : 0, 1214
crashtracking [candidate] (1.2 ms) : 0, 1200
BytebuddyAgent [baseline] (799.241 ms) : 0, 799241
BytebuddyAgent [candidate] (796.263 ms) : 0, 796263
AgentMeter [baseline] (11.249 ms) : 0, 11249
AgentMeter [candidate] (11.36 ms) : 0, 11360
GlobalTracer [baseline] (247.28 ms) : 0, 247280
GlobalTracer [candidate] (248.054 ms) : 0, 248054
IAST [baseline] (26.945 ms) : 0, 26945
IAST [candidate] (27.151 ms) : 0, 27151
AppSec [baseline] (33.954 ms) : 0, 33954
AppSec [candidate] (34.042 ms) : 0, 34042
Debugger [baseline] (66.183 ms) : 0, 66183
Debugger [candidate] (65.557 ms) : 0, 65557
Remote Config [baseline] (556.637 µs) : 0, 557
Remote Config [candidate] (532.111 µs) : 0, 532
Telemetry [baseline] (8.701 ms) : 0, 8701
Telemetry [candidate] (8.568 ms) : 0, 8568
Flare Poller [baseline] (3.502 ms) : 0, 3502
Flare Poller [candidate] (3.475 ms) : 0, 3475
LoadParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 20 metrics, 16 unstable metrics. Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.60.0-SNAPSHOT~5ba371eb8e, baseline=1.60.0-SNAPSHOT~af8b84438c
dateFormat X
axisFormat %s
section baseline
no_agent (1.173 ms) : 1161, 1184
. : milestone, 1173,
iast (3.301 ms) : 3252, 3350
. : milestone, 3301,
iast_FULL (5.663 ms) : 5608, 5718
. : milestone, 5663,
iast_GLOBAL (3.56 ms) : 3498, 3621
. : milestone, 3560,
profiling (2.22 ms) : 2199, 2241
. : milestone, 2220,
tracing (1.79 ms) : 1775, 1805
. : milestone, 1790,
section candidate
no_agent (1.198 ms) : 1186, 1211
. : milestone, 1198,
iast (3.202 ms) : 3160, 3245
. : milestone, 3202,
iast_FULL (5.753 ms) : 5694, 5811
. : milestone, 5753,
iast_GLOBAL (3.558 ms) : 3506, 3609
. : milestone, 3558,
profiling (2.118 ms) : 2099, 2137
. : milestone, 2118,
tracing (1.829 ms) : 1813, 1844
. : milestone, 1829,
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.60.0-SNAPSHOT~5ba371eb8e, baseline=1.60.0-SNAPSHOT~af8b84438c
dateFormat X
axisFormat %s
section baseline
no_agent (18.973 ms) : 18777, 19169
. : milestone, 18973,
appsec (18.458 ms) : 18271, 18646
. : milestone, 18458,
code_origins (17.627 ms) : 17452, 17802
. : milestone, 17627,
iast (17.731 ms) : 17558, 17904
. : milestone, 17731,
profiling (18.468 ms) : 18285, 18651
. : milestone, 18468,
tracing (17.878 ms) : 17700, 18056
. : milestone, 17878,
section candidate
no_agent (19.079 ms) : 18881, 19277
. : milestone, 19079,
appsec (18.594 ms) : 18407, 18781
. : milestone, 18594,
code_origins (17.345 ms) : 17175, 17515
. : milestone, 17345,
iast (17.924 ms) : 17745, 18103
. : milestone, 17924,
profiling (18.313 ms) : 18132, 18495
. : milestone, 18313,
tracing (17.33 ms) : 17161, 17499
. : milestone, 17330,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 11 metrics, 1 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.60.0-SNAPSHOT~5ba371eb8e, baseline=1.60.0-SNAPSHOT~af8b84438c
dateFormat X
axisFormat %s
section baseline
no_agent (1.476 ms) : 1465, 1488
. : milestone, 1476,
appsec (3.796 ms) : 3574, 4018
. : milestone, 3796,
iast (2.245 ms) : 2176, 2314
. : milestone, 2245,
iast_GLOBAL (2.301 ms) : 2232, 2371
. : milestone, 2301,
profiling (2.095 ms) : 2039, 2151
. : milestone, 2095,
tracing (2.053 ms) : 2000, 2106
. : milestone, 2053,
section candidate
no_agent (1.475 ms) : 1464, 1487
. : milestone, 1475,
appsec (3.721 ms) : 3504, 3937
. : milestone, 3721,
iast (2.257 ms) : 2188, 2326
. : milestone, 2257,
iast_GLOBAL (2.289 ms) : 2220, 2358
. : milestone, 2289,
profiling (2.084 ms) : 2029, 2138
. : milestone, 2084,
tracing (2.066 ms) : 2012, 2119
. : milestone, 2066,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.60.0-SNAPSHOT~5ba371eb8e, baseline=1.60.0-SNAPSHOT~af8b84438c
dateFormat X
axisFormat %s
section baseline
no_agent (14.998 s) : 14998000, 14998000
. : milestone, 14998000,
appsec (14.752 s) : 14752000, 14752000
. : milestone, 14752000,
iast (17.89 s) : 17890000, 17890000
. : milestone, 17890000,
iast_GLOBAL (17.946 s) : 17946000, 17946000
. : milestone, 17946000,
profiling (14.999 s) : 14999000, 14999000
. : milestone, 14999000,
tracing (14.698 s) : 14698000, 14698000
. : milestone, 14698000,
section candidate
no_agent (15.338 s) : 15338000, 15338000
. : milestone, 15338000,
appsec (15.015 s) : 15015000, 15015000
. : milestone, 15015000,
iast (18.19 s) : 18190000, 18190000
. : milestone, 18190000,
iast_GLOBAL (17.812 s) : 17812000, 17812000
. : milestone, 17812000,
profiling (14.832 s) : 14832000, 14832000
. : milestone, 14832000,
tracing (14.729 s) : 14729000, 14729000
. : milestone, 14729000,
|
shatzi
left a comment
shatzi
left a comment
There was a problem hiding this comment.
looks great, but I think we need to a little more here
| verify(probeStatusSink, times(1)).addError(probeIdCaptor.capture(), strCaptor.capture()); | ||
| assertEquals(PROBE_ID.getId(), probeIdCaptor.getAllValues().get(0).getId()); | ||
| assertEquals( | ||
| "Instrumentation fails for com.datadog.debugger.MyRecord1", |
There was a problem hiding this comment.
I don’t like this error message. I’d prefer: "Instrumentation is not supported for class.method". This makes it clear to the user that nothing is wrong; it’s simply not supported, rather than implying something failed.
| } | ||
| List<Class<?>> changedClasses = | ||
| finder.getAllLoadedChangedClasses(instrumentation.getAllLoadedClasses(), changes); | ||
| changedClasses = detectMethodParameters(changedClasses); |
There was a problem hiding this comment.
I think this means we filter the transformation for those probes, and therefore we don't report any probe status on those classes.
if this is the case, the UI would say error because no diagnostics where received. How complex would be to report a probe status on that one?
| method.getName(), | ||
| parameters[0].getName()); | ||
| // skip the class: compiled with -parameters | ||
| addClass = false; |
There was a problem hiding this comment.
Idea: if getAllLoadedChangedClasses can return a list of probes per class, we can update their status and avoid rechecking the same class in the future. Since we already know probe-id X is blocked, we do not need to report changes for it.
There was a problem hiding this comment.
Should we bail out for entire applications? If one class was compiled using the -parameters argument is it worth rechecking ever again?
There was a problem hiding this comment.
not all classes will be like this, you can have shared libraries or third-party libs that are not compiled with MethodParameters
jpbempel
force-pushed
the
jpbempel/fix-method-parameters
branch
from
9acdfab to
4bfed3c
Compare
|
Can we add a release note with a breaking change to communicate to customers the added limitations here? |
|
Please note, that on the old JDK-8240908 ticket there has been recent activity for a JDK 17 backport, corresponding See also my related Stack Overflow answer. |
Hi thanks for the comment. I have effectively triggered the backport triggered on the OpenJDK bug you mention. |
|
Hi @jpbempel. I am the one who posted the query in the SO link @kriegaex commented about (SargentD on SO) and I've been trying to get the root cause of the error. Sorry if this is the wrong place to ask this, but I was curious as to why this would behave flaky whenever controller methods with parameters are called? From what we've observed, the issue stops when we restart our server for a while but returns after an hour or so. |
@desmond27 no problem to discuss it here.
hope that helps to understand the issue |
|
@jpbempel That seems plausible. Can we know which version of the Datadog agent was this introduced in (release date as well if possible since we use the https://dtdg.co/latest-java-tracer URL to get the agent)? We've been observing this since 23rd Jan. No issues were observed in the deployment before that time. Edit: This is in the stack trace of the error, if that helps to identify:
Edit 2: One more thing. If Live Debugger were to be enabled in Datadog, could that introduce this issue in an already running application? |
|
@desmond27, Unfortunately, this issue has been part of Exception Replay since its introduction in 1.47.0. In newer tracer versions (1.54.0+), we started enabling the feature by default, which is why you likely began seeing this in January. You can opt out by going to the Error Tracking settings page and disabling Exception Replay for applications affected by this JDK bug until the patch is released. Using Live Debugger on those classes or methods will show the same behavior until the JDK bug is fixed. |
|
Please bear with me, I am going to think aloud:
It is not quite that bad (i.e. exclusive), because instead of In any case, not just the release notes but the main documentation need to clearly describe this limitation and make users aware of the situation and when it can occur - certainly not when classes are transformed via I also suggest to make retransformation optional in cases where we know that the JRE version in question does not support it. Some users might not need the lost parameter names, e.g. because they prefer to have DataDog monitor their Spring contollers at the cost of having to write |
The main problem is that it happens in the middle of nowhere while the application was running fine. Because of the nature of Exception Replay (triggers instrumentation using BTW retransforming classes is also hit by another annoying JVM bug on records JDK-8376185.
We will update the documentation accordingly. Note also that a backport of the patch for this bug is on going for JDK17 that is the minimum version required for Spring 6/Boot 3.
Retransformation is part of the products like Live Debugger, Dynamic Instrumentation, Exception Replay, Code Origin for Spans. Some are manually controlled, others are quite automatic. In any cases you can turn off explicitly all those products. |
|
@jpbempel, yes, I know about the backport. I mentioned it in this thread myself in #10521 (comment). Slightly off-topic: I think Spring could be improved to not throw exceptions but simply use the default JVM parameter names like |
Just because as a non-user I got curious if I could reproduce the problem, I activated Live Debugger and Exception Replay and also made sure that one instrumented controller method also throws a random exception in 50% of the cases. The problem did not occur, as if I was unable to trigger retransformation. Can someone explain to me how I can make sure that retransformation gets triggered? FWIW, my super simple reproducer (no DataDog, no agent on the command line, just plain Java) at Stack Overflow shows that retransformation indeed does cause the loss of method parameter names. |
With Live Debugger: just launch your spring app, not hitting yet an endpoint with parameters. Add a logpoint on that method, hit the endpoint with a request. you should have the exception. |
Brilliant, this works. Thank you so much, @jpbempel. @desmond27, I think we now have the DataDog (DD) reproducer we wanted. The application works normally, then some dynamic instrumentation happens via retransformation, and despite having been compiled with 
Then, I added the DD logpoint: 
Next, I hit the endpoint, seeing an error: The corresponding Spring error log: |
|
Nice. Now all I need to see is when this was enabled in prod on our end and I can close the RCA. Thanks @kriegaex @jpbempel for the insightful comments. Edit: One last question though @jpbempel . Does running a live debug session on Datadog trigger retransformation on all classes? Also, can exceptions encountered during runtime trigger it? I am asking because we faced this in multiple controllers that we didn't run added any log points to. |
Live debugging is manual and only retransform the classes where you put a logpoint/probe. |
Prevent classfiles with Method Parameters attribute (javac -parameters) to be (re)transformed when on JDK < 19. Spring 6+ or SpringBoot3+ rely exclusively on method parameters to get param name and if the attribute is not present throw an exception and returns 500 on endpoints. see OpenJDK bug JDK-8240908. we are scanning method with at least on parameter to detect if the class was compiled with method parameters attribute and if the JDK is < 19 we prevent instrumentation to happen. Even at load time we prevent it because we need to call retransform to remove instrumentation. Therefore the attribute can be strip at that time.
canonical record constructor has method parameters attribute
jpbempel
force-pushed
the
jpbempel/fix-method-parameters
branch
from
f0f6ad1 to
5ba371e
Compare
|
For documentation purposes, I want to share what I have learned in the last 30 minutes when experimenting with Spring Boot 1 and 2, because I was curious if it really worked without
|
5 participants
What Does This Do
Prevent classfiles with Method Parameters attribute (javac -parameters) to be (re)transformed when on JDK < 19. Spring 6+ or SpringBoot3+ rely exclusively on method parameters to get param name and if the attribute is not present throw an exception and returns 500 on endpoints.
see OpenJDK bug JDK-8240908.
we are scanning method with at least on parameter to detect if the class was compiled with method parameters attribute and if the JDK is < 19 we prevent instrumentation to happen.
Even at load time we prevent it because we need to call retransform to remove instrumentation. Therefore the attribute can be strip at that time.
Then we are trying to scan for a specific Spring6+ only class. If the class is not present we are considering this is
safe to retransform as those versions of Spring are not relying on this.
Note: because Scala compiler always emits Method Parameters attribute, probe cannot work on scala if running on JDK < 19
Motivation
Additional Notes
OpenJDK bug
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: DEBUG-5131