-
Notifications
You must be signed in to change notification settings - Fork 323
Rewrite build-time instrumentation as compilation post-processor (using Gradle lazy APIs) #9475
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
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
AlexeyKuznetsov-DD
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.
Just curious, what would be the benefit? Faster build?
248b4eb to
3094d50
Compare
|
Status report The new approach of the instrument post-processing works in almost all simple projects, e.g. Important The diff was made by this custom tool https://github.com/bric3/jardiff
|
aa6d660 to
0067ada
Compare
|
Just a minor comment. Probably it make sense to refactor this plugin to Kotlin. |
0067ada to
210d17c
Compare
d3a2cd2 to
899f804
Compare
The current implementation was using bad practices in various ways, `project.afterEvaluate`, task creation, explicit `dependsOn` # Conflicts: # buildSrc/src/main/groovy/InstrumentPlugin.groovy # buildSrc/src/main/groovy/MuzzlePlugin.groovy # dd-java-agent/instrumentation/jetty-9/build.gradle # Conflicts: # buildSrc/src/main/groovy/InstrumentPlugin.groovy # buildSrc/src/test/groovy/InstrumentPluginTest.groovy # dd-java-agent/instrumentation/play/play-2.4/build.gradle # dd-java-agent/instrumentation/play/play-2.6/build.gradle
…utions to instrument plugin
…and configuration
899f804 to
9b5556c
Compare
Yes, but I'd rather do it, in a second pass. Once this settles down. |
BenchmarksStartupParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 54 metrics, 10 unstable metrics.
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.58.0-SNAPSHOT~5311fd3ea8, baseline=1.58.0~bd3f6f5c89
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.085 s) : 0, 1085410
Total [baseline] (8.777 s) : 0, 8776766
Agent [candidate] (1.095 s) : 0, 1095327
Total [candidate] (8.812 s) : 0, 8812288
section iast
Agent [baseline] (1.227 s) : 0, 1226795
Total [baseline] (9.373 s) : 0, 9373371
Agent [candidate] (1.233 s) : 0, 1232838
Total [candidate] (9.339 s) : 0, 9339380
gantt
title insecure-bank - break down per module: candidate=1.58.0-SNAPSHOT~5311fd3ea8, baseline=1.58.0~bd3f6f5c89
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.201 ms) : 0, 1201
crashtracking [candidate] (1.204 ms) : 0, 1204
BytebuddyAgent [baseline] (651.831 ms) : 0, 651831
BytebuddyAgent [candidate] (657.349 ms) : 0, 657349
GlobalTracer [baseline] (282.883 ms) : 0, 282883
GlobalTracer [candidate] (285.87 ms) : 0, 285870
AppSec [baseline] (32.731 ms) : 0, 32731
AppSec [candidate] (33.079 ms) : 0, 33079
Debugger [baseline] (67.728 ms) : 0, 67728
Debugger [candidate] (68.475 ms) : 0, 68475
Remote Config [baseline] (642.001 µs) : 0, 642
Remote Config [candidate] (629.818 µs) : 0, 630
Telemetry [baseline] (9.009 ms) : 0, 9009
Telemetry [candidate] (9.093 ms) : 0, 9093
Flare Poller [baseline] (3.762 ms) : 0, 3762
Flare Poller [candidate] (3.759 ms) : 0, 3759
section iast
crashtracking [baseline] (1.194 ms) : 0, 1194
crashtracking [candidate] (1.188 ms) : 0, 1188
BytebuddyAgent [baseline] (793.543 ms) : 0, 793543
BytebuddyAgent [candidate] (796.776 ms) : 0, 796776
GlobalTracer [baseline] (256.574 ms) : 0, 256574
GlobalTracer [candidate] (258.959 ms) : 0, 258959
AppSec [baseline] (34.553 ms) : 0, 34553
AppSec [candidate] (33.897 ms) : 0, 33897
Debugger [baseline] (65.38 ms) : 0, 65380
Debugger [candidate] (66.295 ms) : 0, 66295
Remote Config [baseline] (601.877 µs) : 0, 602
Remote Config [candidate] (593.286 µs) : 0, 593
Telemetry [baseline] (8.591 ms) : 0, 8591
Telemetry [candidate] (8.548 ms) : 0, 8548
Flare Poller [baseline] (3.669 ms) : 0, 3669
Flare Poller [candidate] (3.622 ms) : 0, 3622
IAST [baseline] (27.139 ms) : 0, 27139
IAST [candidate] (27.342 ms) : 0, 27342
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.58.0-SNAPSHOT~5311fd3ea8, baseline=1.58.0~bd3f6f5c89
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.103 s) : 0, 1102909
Total [baseline] (10.822 s) : 0, 10821666
Agent [candidate] (1.086 s) : 0, 1086349
Total [candidate] (10.835 s) : 0, 10834565
section appsec
Agent [baseline] (1.266 s) : 0, 1265820
Total [baseline] (10.977 s) : 0, 10976557
Agent [candidate] (1.291 s) : 0, 1290887
Total [candidate] (11.023 s) : 0, 11023422
section iast
Agent [baseline] (1.235 s) : 0, 1235492
Total [baseline] (11.3 s) : 0, 11300205
Agent [candidate] (1.243 s) : 0, 1242797
Total [candidate] (11.243 s) : 0, 11242785
section profiling
Agent [baseline] (1.207 s) : 0, 1207159
Total [baseline] (10.953 s) : 0, 10953459
Agent [candidate] (1.216 s) : 0, 1216212
Total [candidate] (11.004 s) : 0, 11003573
gantt
title petclinic - break down per module: candidate=1.58.0-SNAPSHOT~5311fd3ea8, baseline=1.58.0~bd3f6f5c89
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.209 ms) : 0, 1209
crashtracking [candidate] (1.18 ms) : 0, 1180
BytebuddyAgent [baseline] (662.931 ms) : 0, 662931
BytebuddyAgent [candidate] (651.819 ms) : 0, 651819
GlobalTracer [baseline] (286.712 ms) : 0, 286712
GlobalTracer [candidate] (283.198 ms) : 0, 283198
AppSec [baseline] (33.222 ms) : 0, 33222
AppSec [candidate] (32.666 ms) : 0, 32666
Debugger [baseline] (69.168 ms) : 0, 69168
Debugger [candidate] (68.51 ms) : 0, 68510
Remote Config [baseline] (645.39 µs) : 0, 645
Remote Config [candidate] (614.18 µs) : 0, 614
Telemetry [baseline] (9.166 ms) : 0, 9166
Telemetry [candidate] (9.031 ms) : 0, 9031
Flare Poller [baseline] (3.77 ms) : 0, 3770
Flare Poller [candidate] (3.74 ms) : 0, 3740
section appsec
crashtracking [baseline] (1.183 ms) : 0, 1183
crashtracking [candidate] (1.2 ms) : 0, 1200
BytebuddyAgent [baseline] (690.618 ms) : 0, 690618
BytebuddyAgent [candidate] (706.716 ms) : 0, 706716
GlobalTracer [baseline] (258.163 ms) : 0, 258163
GlobalTracer [candidate] (264.087 ms) : 0, 264087
AppSec [baseline] (174.866 ms) : 0, 174866
AppSec [candidate] (176.774 ms) : 0, 176774
Debugger [baseline] (66.937 ms) : 0, 66937
Debugger [candidate] (66.845 ms) : 0, 66845
Remote Config [baseline] (770.227 µs) : 0, 770
Remote Config [candidate] (751.551 µs) : 0, 752
Telemetry [baseline] (9.486 ms) : 0, 9486
Telemetry [candidate] (9.497 ms) : 0, 9497
Flare Poller [baseline] (3.792 ms) : 0, 3792
Flare Poller [candidate] (3.76 ms) : 0, 3760
IAST [baseline] (24.544 ms) : 0, 24544
IAST [candidate] (25.358 ms) : 0, 25358
section iast
crashtracking [baseline] (1.19 ms) : 0, 1190
crashtracking [candidate] (1.201 ms) : 0, 1201
BytebuddyAgent [baseline] (798.331 ms) : 0, 798331
BytebuddyAgent [candidate] (804.609 ms) : 0, 804609
GlobalTracer [baseline] (258.354 ms) : 0, 258354
GlobalTracer [candidate] (259.866 ms) : 0, 259866
AppSec [baseline] (32.08 ms) : 0, 32080
AppSec [candidate] (35.63 ms) : 0, 35630
Debugger [baseline] (69.608 ms) : 0, 69608
Debugger [candidate] (65.227 ms) : 0, 65227
Remote Config [baseline] (555.089 µs) : 0, 555
Remote Config [candidate] (569.906 µs) : 0, 570
Telemetry [baseline] (8.618 ms) : 0, 8618
Telemetry [candidate] (8.627 ms) : 0, 8627
Flare Poller [baseline] (3.716 ms) : 0, 3716
Flare Poller [candidate] (3.643 ms) : 0, 3643
IAST [baseline] (27.372 ms) : 0, 27372
IAST [candidate] (27.538 ms) : 0, 27538
section profiling
crashtracking [baseline] (1.23 ms) : 0, 1230
crashtracking [candidate] (1.225 ms) : 0, 1225
BytebuddyAgent [baseline] (704.48 ms) : 0, 704480
BytebuddyAgent [candidate] (710.637 ms) : 0, 710637
GlobalTracer [baseline] (220.713 ms) : 0, 220713
GlobalTracer [candidate] (222.6 ms) : 0, 222600
AppSec [baseline] (32.135 ms) : 0, 32135
AppSec [candidate] (32.193 ms) : 0, 32193
Debugger [baseline] (68.249 ms) : 0, 68249
Debugger [candidate] (68.034 ms) : 0, 68034
Remote Config [baseline] (664.737 µs) : 0, 665
Remote Config [candidate] (639.971 µs) : 0, 640
Telemetry [baseline] (8.734 ms) : 0, 8734
Telemetry [candidate] (8.913 ms) : 0, 8913
Flare Poller [baseline] (3.641 ms) : 0, 3641
Flare Poller [candidate] (3.673 ms) : 0, 3673
ProfilingAgent [baseline] (97.346 ms) : 0, 97346
ProfilingAgent [candidate] (97.817 ms) : 0, 97817
Profiling [baseline] (97.915 ms) : 0, 97915
Profiling [candidate] (98.388 ms) : 0, 98388
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 0 performance regressions! Performance is the same for 19 metrics, 16 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.58.0-SNAPSHOT~5311fd3ea8, baseline=1.58.0~bd3f6f5c89
dateFormat X
axisFormat %s
section baseline
no_agent (18.143 ms) : 17956, 18330
. : milestone, 18143,
appsec (18.676 ms) : 18485, 18868
. : milestone, 18676,
code_origins (17.591 ms) : 17419, 17763
. : milestone, 17591,
iast (17.861 ms) : 17682, 18041
. : milestone, 17861,
profiling (18.854 ms) : 18664, 19043
. : milestone, 18854,
tracing (18.143 ms) : 17964, 18323
. : milestone, 18143,
section candidate
no_agent (18.355 ms) : 18170, 18541
. : milestone, 18355,
appsec (19.15 ms) : 18958, 19341
. : milestone, 19150,
code_origins (17.603 ms) : 17429, 17778
. : milestone, 17603,
iast (17.802 ms) : 17625, 17978
. : milestone, 17802,
profiling (18.553 ms) : 18370, 18735
. : milestone, 18553,
tracing (17.808 ms) : 17630, 17986
. : milestone, 17808,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.58.0-SNAPSHOT~5311fd3ea8, baseline=1.58.0~bd3f6f5c89
dateFormat X
axisFormat %s
section baseline
no_agent (1.197 ms) : 1185, 1208
. : milestone, 1197,
iast (3.166 ms) : 3123, 3209
. : milestone, 3166,
iast_FULL (5.587 ms) : 5533, 5641
. : milestone, 5587,
iast_GLOBAL (3.65 ms) : 3582, 3719
. : milestone, 3650,
profiling (2.078 ms) : 2060, 2096
. : milestone, 2078,
tracing (1.916 ms) : 1897, 1935
. : milestone, 1916,
section candidate
no_agent (1.189 ms) : 1177, 1201
. : milestone, 1189,
iast (3.154 ms) : 3115, 3193
. : milestone, 3154,
iast_FULL (5.623 ms) : 5567, 5679
. : milestone, 5623,
iast_GLOBAL (3.654 ms) : 3591, 3716
. : milestone, 3654,
profiling (2.076 ms) : 2057, 2095
. : milestone, 2076,
tracing (1.775 ms) : 1760, 1790
. : milestone, 1775,
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.58.0-SNAPSHOT~5311fd3ea8, baseline=1.58.0~bd3f6f5c89
dateFormat X
axisFormat %s
section baseline
no_agent (1.476 ms) : 1464, 1487
. : milestone, 1476,
appsec (3.744 ms) : 3523, 3964
. : milestone, 3744,
iast (2.22 ms) : 2155, 2285
. : milestone, 2220,
iast_GLOBAL (2.264 ms) : 2199, 2330
. : milestone, 2264,
profiling (2.098 ms) : 2043, 2153
. : milestone, 2098,
tracing (2.046 ms) : 1995, 2097
. : milestone, 2046,
section candidate
no_agent (1.479 ms) : 1468, 1491
. : milestone, 1479,
appsec (3.748 ms) : 3526, 3970
. : milestone, 3748,
iast (2.224 ms) : 2158, 2289
. : milestone, 2224,
iast_GLOBAL (2.253 ms) : 2188, 2318
. : milestone, 2253,
profiling (2.072 ms) : 2019, 2125
. : milestone, 2072,
tracing (2.053 ms) : 2002, 2104
. : milestone, 2053,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.58.0-SNAPSHOT~5311fd3ea8, baseline=1.58.0~bd3f6f5c89
dateFormat X
axisFormat %s
section baseline
no_agent (15.441 s) : 15441000, 15441000
. : milestone, 15441000,
appsec (14.603 s) : 14603000, 14603000
. : milestone, 14603000,
iast (17.736 s) : 17736000, 17736000
. : milestone, 17736000,
iast_GLOBAL (17.733 s) : 17733000, 17733000
. : milestone, 17733000,
profiling (15.03 s) : 15030000, 15030000
. : milestone, 15030000,
tracing (14.508 s) : 14508000, 14508000
. : milestone, 14508000,
section candidate
no_agent (15.538 s) : 15538000, 15538000
. : milestone, 15538000,
appsec (14.76 s) : 14760000, 14760000
. : milestone, 14760000,
iast (18.134 s) : 18134000, 18134000
. : milestone, 18134000,
iast_GLOBAL (17.696 s) : 17696000, 17696000
. : milestone, 17696000,
profiling (14.758 s) : 14758000, 14758000
. : milestone, 14758000,
tracing (14.582 s) : 14582000, 14582000
. : milestone, 14582000,
|
InstrumentPlugin as compilation post-processor and lazy
a485d08 to
83aa2a4
Compare
InstrumentPlugin as compilation post-processor and lazy83aa2a4 to
5311fd3
Compare
AlexeyKuznetsov-DD
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.
LGTM, left minor notes and questions.
buildSrc/src/main/groovy/datadog/gradle/plugin/instrument/BuildTimeInstrumentationPlugin.groovy
Show resolved
Hide resolved
| logger.info('[BuildTimeInstrumentationPlugin] Applying buildTimeInstrumentationPlugin configuration as compile task input') | ||
| it.inputs.files(project.configurations.named(BUILD_TIME_INSTRUMENTATION_PLUGIN_CONFIGURATION)) | ||
|
|
||
| if (it.source.isEmpty()) { |
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.
Do we need this check? if it was already checked on line 103?
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.
Indeed, maybe this can be removed.
| it.inputs.property("javaVersion", javaVersion) | ||
|
|
||
| it.inputs.property("plugins", extension.plugins) | ||
|
|
||
| it.inputs.files(extension.additionalClasspath) |
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.
Minor - remove empty lines to keep lines logically together.
| it.inputs.property("javaVersion", javaVersion) | |
| it.inputs.property("plugins", extension.plugins) | |
| it.inputs.files(extension.additionalClasspath) | |
| it.inputs.property("javaVersion", javaVersion) | |
| it.inputs.property("plugins", extension.plugins) | |
| it.inputs.files(extension.additionalClasspath) |
| tmpUninstrumentedClasses | ||
| ) | ||
|
|
||
| // This were the post processing happens, i.e. were the instrumentation is applied |
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.
minor: I would re-phrase this comment a bit, probably make it short: // Apply instrumentation.
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 were the post processing happens, i.e. were the instrumentation is applied | |
| // Post-process classes with instrumentation |
buildSrc/src/main/groovy/datadog/gradle/plugin/instrument/ByteBuddyInstrumenter.groovy
Show resolved
Hide resolved
| logger.info( | ||
| "[InstrumentPostProcessingAction] About to instrument classes \n" + | ||
| " javaVersion=${javaVersion}, \n" + | ||
| " plugins=${plugins.get()}, \n" + | ||
| " instrumentingClassPath=${instrumentingClassPath.files}, \n" + | ||
| " rawClassesDirectory=${compilerOutputDirectory.get().asFile}" | ||
| ) |
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.
minor: can we use multiline string here?
| logger.info('[BuildTimeInstrumentationPlugin] Applying buildTimeInstrumentationPlugin configuration as compile task input') | ||
| it.inputs.files(project.configurations.named(BUILD_TIME_INSTRUMENTATION_PLUGIN_CONFIGURATION)) | ||
|
|
||
| if (it.source.isEmpty()) { |
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.
Indeed, maybe this can be removed.
| tmpUninstrumentedClasses | ||
| ) | ||
|
|
||
| // This were the post processing happens, i.e. were the instrumentation is applied |
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 were the post processing happens, i.e. were the instrumentation is applied | |
| // Post-process classes with instrumentation |
| it.dependencies.add(subProj.dependencies.project(path: ':dd-java-agent:agent-tooling', configuration: 'instrumentPluginClasspath')) | ||
| it.dependencies.add(subProj.dependencies.project( | ||
| path: ':dd-java-agent:agent-tooling', | ||
| configuration: 'buildTimeInstrumentationPlugin' |
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.
note: Maybe I'll rename this configuration to avoid confusion with the instrumentation plugin.
Something like
configuration: 'buildTimeInstrumentationPlugin'
configuration: 'buildTimeInstrumentationToolingPlugins'

What Does This Do
Disclaimer: This only touches the build part (i.e. Gradle).
The current
InstrumentPluginuses eager gradle API and inject a task to run after the compile task, and in doing so, exchanging their output. This approach is uncommon, create as many instrument task as they are compile tasks, and make it harder to predict compilation output.This PR rethink the approach to a compilation post-processor. So instead of injecting a task in the gradle task graph, each compile task have a post-processor action.
Also, the plugin is renamed to
dd-trace-java.build-time-instrumentationfor better expressing intent. (Related configuration points are also renamed),In doing so, it makes the intent of this plug-in easier to grasp.
On another note, the single Groovy file has been spread to distinct types.
Motivation
dd-trace-java.build-time-instrumentationwas chosen to aligndd-trace-java.call-site-instrumentation. Also, searching these string will bring more focused results.Avoid Gradle eager API, avoid messing with compile tasks avoid,
Helps going toward convention plug-ins.
Additional Notes
Related PRs
Related to
MuzzlePlugin#9315CallSiteInstrumentationPlugin#9316Blocked by
afterEvaluate#9752testJvmConstraintsGradle extension to replace extra properties #9892:dd-java-agent:testing#10218jetty-server-9.0project setup to multiple modules #9683What does this plugin
ℹ️ italic word indicate Gradle linguo.
buildTimeInstrumentationname (previouslyinstrument)buildTimeInstrumentationPluginname (previouslyinstrumentPluginClasspath)Currently, this build time instrumentation is applied on instrumentation modules :
datadog.trace.agent.tooling.muzzle.MuzzleGradlePlugin: Creates muzzle-references at compile time for classes extendingInstrumenterModule. This generates metadata about instrumentation compatibility.datadog.trace.agent.tooling.bytebuddy.NewTaskForGradlePlugin: Used fordatadog.trace.instrumentation.java.concurrent.WrapRunnableAsNewTaskInstrumentationinstrumentation, to replacedatadog.trace.bootstrap.instrumentation.java.concurrent.NewTaskForPlaceholder#newTaskForbyjava.util.concurrent.AbstractExecutorService#newTaskFor(java.lang.Runnable, T)(which is protected and not accessible during compilation).datadog.trace.agent.tooling.bytebuddy.reqctx.RewriteRequestContextAdvicePlugin: Transforms classes annotated with @RequiresRequestContext to inject request context handling....and in the otel agent
datadog.opentelemetry.tooling.shim.OtelShimGradlePlugin: Injects Datadog's OpenTelemetry shim into specific OpenTelemetry API classes like DefaultOpenTelemetry, GlobalOpenTelemetry$ObfuscatedOpenTelemetry, ThreadLocalContextStorage, etc. This allows intercepting OpenTelemetry API calls at build-time.Additional checks
As in #9514 I also checked the produced jars between
masterand this branch using myjardifftool I already mentioned there (I crafted it for validating #9514, other tools didn't work or didn't perform what was needed).Note
🔗 https://github.com/bric3/jardiff
Shortcomings:
Ran the following command on this branch (folder
dd-trace-java-copy-2), and onmaster(folderdd-trace-java-copy-1).The following assumes that
jardiffis actually this commandjava -jar jardiff/build/libs/jardiff-0.1.0-SNAPSHOT.jar, and assuming this is a Java 25. Also, while not strictly necessary the following snippets usedeltato render the diff.The following difference have been checked:
dd-java-agentjardiff ../dd-trace-java-copy-{1,2}/dd-java-agent/build/libs/dd-java-agent-1.58.0-SNAPSHOT.jar \ -c classdata \ --exclude "**/*.yaml,**/*.MF,**/*.version,*.version,*.txt" \ | delta --syntax-theme=GitHub --lightNo differences: