-
Notifications
You must be signed in to change notification settings - Fork 312
Introducing NativeLoader #9625
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?
Introducing NativeLoader #9625
Conversation
Introducing NativeLoader API intended to standardize how native libraries are loaded in dd-trace-java NativeLoader allows for using different file layout and file resolution strategies
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
components/nativeloader/src/test/java/datadog/nativeloader/NativeLoaderTest.java
Outdated
Show resolved
Hide resolved
Switched Objects.hash -> Arrays.hashCode
🎯 Code Coverage 🔗 Commit SHA: 8401635 | Docs | Was this helpful? Give us feedback! |
} | ||
|
||
@Override | ||
public boolean isArm32() { |
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.
Need to decide how to fully connect the arch detection logic
Might want to move the arch detection logic from profiling into components:environment
/** | ||
* Loads a library associated with an associated component | ||
*/ | ||
public final void load(String component, String libName) throws LibraryLoadException { |
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.
NativeLoader has a notion of component, so we can keep our component structure inside the JAR. The intention is that we can create a custom LibraryResolver that routes based on component as we see fit.
components/native-loader/src/main/java/datadog/nativeloader/NativeLoader.java
Outdated
Show resolved
Hide resolved
BenchmarksStartupParameters
See matching parameters
SummaryFound 6 performance improvements and 0 performance regressions! Performance is the same for 52 metrics, 7 unstable metrics.
Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.54.0-SNAPSHOT~8401635306, baseline=1.54.0-SNAPSHOT~70b85a8492
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.019 s) : 0, 1018782
Total [baseline] (10.753 s) : 0, 10752509
Agent [candidate] (1.026 s) : 0, 1025959
Total [candidate] (10.672 s) : 0, 10671979
section appsec
Agent [baseline] (1.2 s) : 0, 1199694
Total [baseline] (11.06 s) : 0, 11059717
Agent [candidate] (1.191 s) : 0, 1190612
Total [candidate] (9.765 s) : 0, 9765258
section iast
Agent [baseline] (1.153 s) : 0, 1153107
Total [baseline] (10.923 s) : 0, 10923344
Agent [candidate] (1.153 s) : 0, 1153432
Total [candidate] (11.079 s) : 0, 11078739
section profiling
Agent [baseline] (1.171 s) : 0, 1170879
Total [baseline] (11.036 s) : 0, 11036295
Agent [candidate] (1.154 s) : 0, 1153611
Total [candidate] (10.947 s) : 0, 10947325
gantt
title petclinic - break down per module: candidate=1.54.0-SNAPSHOT~8401635306, baseline=1.54.0-SNAPSHOT~70b85a8492
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.46 ms) : 0, 1460
crashtracking [candidate] (1.47 ms) : 0, 1470
BytebuddyAgent [baseline] (693.555 ms) : 0, 693555
BytebuddyAgent [candidate] (699.079 ms) : 0, 699079
GlobalTracer [baseline] (243.042 ms) : 0, 243042
GlobalTracer [candidate] (245.125 ms) : 0, 245125
AppSec [baseline] (32.471 ms) : 0, 32471
AppSec [candidate] (31.559 ms) : 0, 31559
Debugger [baseline] (6.338 ms) : 0, 6338
Debugger [candidate] (6.459 ms) : 0, 6459
Remote Config [baseline] (674.932 µs) : 0, 675
Remote Config [candidate] (699.435 µs) : 0, 699
Telemetry [baseline] (9.247 ms) : 0, 9247
Telemetry [candidate] (9.013 ms) : 0, 9013
Flare Poller [baseline] (10.789 ms) : 0, 10789
Flare Poller [candidate] (11.135 ms) : 0, 11135
section appsec
crashtracking [baseline] (1.49 ms) : 0, 1490
crashtracking [candidate] (1.453 ms) : 0, 1453
BytebuddyAgent [baseline] (722.52 ms) : 0, 722520
BytebuddyAgent [candidate] (715.872 ms) : 0, 715872
GlobalTracer [baseline] (236.104 ms) : 0, 236104
GlobalTracer [candidate] (235.079 ms) : 0, 235079
IAST [baseline] (24.745 ms) : 0, 24745
IAST [candidate] (24.554 ms) : 0, 24554
AppSec [baseline] (173.726 ms) : 0, 173726
AppSec [candidate] (171.79 ms) : 0, 171790
Debugger [baseline] (6.065 ms) : 0, 6065
Debugger [candidate] (6.07 ms) : 0, 6070
Remote Config [baseline] (643.139 µs) : 0, 643
Remote Config [candidate] (637.017 µs) : 0, 637
Telemetry [baseline] (9.249 ms) : 0, 9249
Telemetry [candidate] (9.849 ms) : 0, 9849
Flare Poller [baseline] (3.916 ms) : 0, 3916
Flare Poller [candidate] (4.02 ms) : 0, 4020
section iast
crashtracking [baseline] (1.471 ms) : 0, 1471
crashtracking [candidate] (1.456 ms) : 0, 1456
BytebuddyAgent [baseline] (815.678 ms) : 0, 815678
BytebuddyAgent [candidate] (816.218 ms) : 0, 816218
GlobalTracer [baseline] (233.6 ms) : 0, 233600
GlobalTracer [candidate] (234.24 ms) : 0, 234240
IAST [baseline] (26.356 ms) : 0, 26356
IAST [candidate] (26.525 ms) : 0, 26525
AppSec [baseline] (35.256 ms) : 0, 35256
AppSec [candidate] (34.323 ms) : 0, 34323
Debugger [baseline] (6.138 ms) : 0, 6138
Debugger [candidate] (6.089 ms) : 0, 6089
Remote Config [baseline] (588.587 µs) : 0, 589
Remote Config [candidate] (593.678 µs) : 0, 594
Telemetry [baseline] (8.571 ms) : 0, 8571
Telemetry [candidate] (8.164 ms) : 0, 8164
Flare Poller [baseline] (4.162 ms) : 0, 4162
Flare Poller [candidate] (4.281 ms) : 0, 4281
section profiling
crashtracking [baseline] (1.428 ms) : 0, 1428
crashtracking [candidate] (1.415 ms) : 0, 1415
BytebuddyAgent [baseline] (726.667 ms) : 0, 726667
BytebuddyAgent [candidate] (719.757 ms) : 0, 719757
GlobalTracer [baseline] (220.089 ms) : 0, 220089
GlobalTracer [candidate] (218.089 ms) : 0, 218089
AppSec [baseline] (33.238 ms) : 0, 33238
AppSec [candidate] (31.155 ms) : 0, 31155
Debugger [baseline] (7.297 ms) : 0, 7297
Debugger [candidate] (7.248 ms) : 0, 7248
Remote Config [baseline] (785.991 µs) : 0, 786
Remote Config [candidate] (727.196 µs) : 0, 727
Telemetry [baseline] (16.018 ms) : 0, 16018
Telemetry [candidate] (15.547 ms) : 0, 15547
Flare Poller [baseline] (4.192 ms) : 0, 4192
Flare Poller [candidate] (4.183 ms) : 0, 4183
ProfilingAgent [baseline] (108.228 ms) : 0, 108228
ProfilingAgent [candidate] (103.126 ms) : 0, 103126
Profiling [baseline] (109.075 ms) : 0, 109075
Profiling [candidate] (103.717 ms) : 0, 103717
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.54.0-SNAPSHOT~8401635306, baseline=1.54.0-SNAPSHOT~70b85a8492
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.027 s) : 0, 1027080
Total [baseline] (8.669 s) : 0, 8669041
Agent [candidate] (1.012 s) : 0, 1011749
Total [candidate] (8.675 s) : 0, 8675248
section iast
Agent [baseline] (1.153 s) : 0, 1152803
Total [baseline] (9.243 s) : 0, 9243390
Agent [candidate] (1.145 s) : 0, 1145086
Total [candidate] (9.26 s) : 0, 9260489
gantt
title insecure-bank - break down per module: candidate=1.54.0-SNAPSHOT~8401635306, baseline=1.54.0-SNAPSHOT~70b85a8492
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.483 ms) : 0, 1483
crashtracking [candidate] (1.453 ms) : 0, 1453
BytebuddyAgent [baseline] (700.298 ms) : 0, 700298
BytebuddyAgent [candidate] (688.734 ms) : 0, 688734
GlobalTracer [baseline] (244.383 ms) : 0, 244383
GlobalTracer [candidate] (242.605 ms) : 0, 242605
AppSec [baseline] (32.796 ms) : 0, 32796
AppSec [candidate] (31.509 ms) : 0, 31509
Debugger [baseline] (6.45 ms) : 0, 6450
Debugger [candidate] (6.37 ms) : 0, 6370
Remote Config [baseline] (691.496 µs) : 0, 691
Remote Config [candidate] (674.708 µs) : 0, 675
Telemetry [baseline] (9.428 ms) : 0, 9428
Telemetry [candidate] (9.097 ms) : 0, 9097
Flare Poller [baseline] (10.24 ms) : 0, 10240
Flare Poller [candidate] (10.186 ms) : 0, 10186
section iast
crashtracking [baseline] (1.481 ms) : 0, 1481
crashtracking [candidate] (1.45 ms) : 0, 1450
BytebuddyAgent [baseline] (815.603 ms) : 0, 815603
BytebuddyAgent [candidate] (811.087 ms) : 0, 811087
GlobalTracer [baseline] (233.193 ms) : 0, 233193
GlobalTracer [candidate] (232.173 ms) : 0, 232173
AppSec [baseline] (35.275 ms) : 0, 35275
AppSec [candidate] (33.908 ms) : 0, 33908
Debugger [baseline] (6.196 ms) : 0, 6196
Debugger [candidate] (6.027 ms) : 0, 6027
Remote Config [baseline] (588.072 µs) : 0, 588
Remote Config [candidate] (573.339 µs) : 0, 573
Telemetry [baseline] (8.559 ms) : 0, 8559
Telemetry [candidate] (8.143 ms) : 0, 8143
Flare Poller [baseline] (4.212 ms) : 0, 4212
Flare Poller [candidate] (4.28 ms) : 0, 4280
IAST [baseline] (26.372 ms) : 0, 26372
IAST [candidate] (26.06 ms) : 0, 26060
LoadParameters
See matching parameters
SummaryFound 1 performance improvements and 3 performance regressions! Performance is the same for 8 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~8401635306, baseline=1.54.0-SNAPSHOT~70b85a8492
dateFormat X
axisFormat %s
section baseline
no_agent (38.39 ms) : 38082, 38699
. : milestone, 38390,
appsec (47.778 ms) : 47341, 48214
. : milestone, 47778,
code_origins (45.404 ms) : 45011, 45796
. : milestone, 45404,
iast (45.086 ms) : 44697, 45476
. : milestone, 45086,
profiling (47.582 ms) : 47165, 47998
. : milestone, 47582,
tracing (43.961 ms) : 43589, 44333
. : milestone, 43961,
section candidate
no_agent (38.093 ms) : 37794, 38393
. : milestone, 38093,
appsec (47.461 ms) : 47044, 47878
. : milestone, 47461,
code_origins (44.189 ms) : 43811, 44566
. : milestone, 44189,
iast (45.727 ms) : 45335, 46119
. : milestone, 45727,
profiling (48.265 ms) : 47802, 48728
. : milestone, 48265,
tracing (44.776 ms) : 44399, 45153
. : milestone, 44776,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~8401635306, baseline=1.54.0-SNAPSHOT~70b85a8492
dateFormat X
axisFormat %s
section baseline
no_agent (4.227 ms) : 4175, 4279
. : milestone, 4227,
iast (9.643 ms) : 9484, 9802
. : milestone, 9643,
iast_FULL (14.043 ms) : 13767, 14318
. : milestone, 14043,
iast_GLOBAL (11.478 ms) : 11268, 11688
. : milestone, 11478,
profiling (8.803 ms) : 8666, 8940
. : milestone, 8803,
tracing (8.076 ms) : 7960, 8193
. : milestone, 8076,
section candidate
no_agent (4.375 ms) : 4324, 4427
. : milestone, 4375,
iast (10.219 ms) : 10046, 10392
. : milestone, 10219,
iast_FULL (14.503 ms) : 14212, 14794
. : milestone, 14503,
iast_GLOBAL (10.273 ms) : 10085, 10462
. : milestone, 10273,
profiling (9.195 ms) : 9043, 9347
. : milestone, 9195,
tracing (7.973 ms) : 7844, 8103
. : milestone, 7973,
DacapoParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 2 unstable metrics. Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~8401635306, baseline=1.54.0-SNAPSHOT~70b85a8492
dateFormat X
axisFormat %s
section baseline
no_agent (1.482 ms) : 1470, 1494
. : milestone, 1482,
appsec (3.68 ms) : 3466, 3894
. : milestone, 3680,
iast (2.226 ms) : 2161, 2290
. : milestone, 2226,
iast_GLOBAL (2.264 ms) : 2199, 2328
. : milestone, 2264,
profiling (2.074 ms) : 2022, 2126
. : milestone, 2074,
tracing (2.047 ms) : 1996, 2098
. : milestone, 2047,
section candidate
no_agent (1.482 ms) : 1471, 1494
. : milestone, 1482,
appsec (3.756 ms) : 3537, 3976
. : milestone, 3756,
iast (2.215 ms) : 2151, 2278
. : milestone, 2215,
iast_GLOBAL (2.262 ms) : 2198, 2327
. : milestone, 2262,
profiling (2.513 ms) : 2345, 2680
. : milestone, 2513,
tracing (2.04 ms) : 1990, 2090
. : milestone, 2040,
Execution time for biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~8401635306, baseline=1.54.0-SNAPSHOT~70b85a8492
dateFormat X
axisFormat %s
section baseline
no_agent (15.632 s) : 15632000, 15632000
. : milestone, 15632000,
appsec (15.01 s) : 15010000, 15010000
. : milestone, 15010000,
iast (18.562 s) : 18562000, 18562000
. : milestone, 18562000,
iast_GLOBAL (18.133 s) : 18133000, 18133000
. : milestone, 18133000,
profiling (15.409 s) : 15409000, 15409000
. : milestone, 15409000,
tracing (14.986 s) : 14986000, 14986000
. : milestone, 14986000,
section candidate
no_agent (15.41 s) : 15410000, 15410000
. : milestone, 15410000,
appsec (15.265 s) : 15265000, 15265000
. : milestone, 15265000,
iast (18.522 s) : 18522000, 18522000
. : milestone, 18522000,
iast_GLOBAL (17.991 s) : 17991000, 17991000
. : milestone, 17991000,
profiling (15.82 s) : 15820000, 15820000
. : milestone, 15820000,
tracing (15.171 s) : 15171000, 15171000
. : milestone, 15171000,
|
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.
That looks promising 👍
👏 praise: It's nice to see that the only dependency to another component was abstracted in a dedicated class (PlatformSpec
)
🔍 nitpick: I would annotation functional interface with @FunctionalInterface
like PathLocator
, LibraryResolver
🔍 nitpick: I wonder if the component module should be :native-loader
rather than :nativeloader
as most of the other modules of the code base (especially around agent).
/** | ||
* Resolves a library using a different {@link PlatformSpec} than the default for this {@link NativeLoader} | ||
*/ | ||
public LibFile resolveDynamic(PlatformSpec platformSpec, String libName) |
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.
❔ question: What would be the use case to load a library for a different plaform spec?
Will that be that common? If not, this could be address by instantiating another loader for this specific spec.
This would simplify a bit the API.
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.
The thinking was that it was useful for finding libraries if we have some sort of installer mode or need to find static libs for musl for Graal AOT build, etc. Although, I'll admit I'm not quite sure how that part is going to work.
components/native-loader/src/main/java/datadog/nativeloader/NativeLoader.java
Show resolved
Hide resolved
url = pathLocator.locate(component, regularPath + "/" + libFileName); | ||
if (url != null) return url; | ||
|
||
// fallback to searching at top-level, mostly concession to good out-of-box behavior | ||
// with java.library.path | ||
url = pathLocator.locate(component, libFileName); | ||
if (url != null) return url; | ||
|
||
if (component != null) { | ||
url = pathLocator.locate(null, libFileName); | ||
if (url != null) return url; | ||
} | ||
|
||
return null; |
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: This look up is duplicate betwenne flat and nested resolver. What about introducing an abstract resolver to capture it? (or whatever static private method to avoid duplication)
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.
Yeah, that's a fair point and one that I'm considering.
I didn't want too many layers to navigate through, but it would be a bit annoying to create a LibraryResolver and have to duplicate all that logic.
And in part, I'm expecting that we'll just end up with a custom LibraryResolver for dd-trace-java, so I didn't want to invest too much here.
components/native-loader/src/main/java/datadog/nativeloader/LibraryResolvers.java
Show resolved
Hide resolved
Enabling the NativeLoader tests - using dummy lib files, but enough to test resolution process and linkage failures More Javadoc to help explain the various pluggable bits of the API
Changed NativeLoader to raise LibraryLoadException rather than returning null when it encounters an unsupported OS or architecture Allowing LibraryResolver and PathLocator to both raise Exceptions Creating PathLocatorHelper to help with exception handling in LibraryResolver-s when multiple locator requests are being performed
Clean-up to account for prior name change of PathResolver to PathLocator
Covering multi-dir case
More coverage of LibDirBasedPathLocator - including component support
Covering DefaultPlatformSpec - now renamed IntrospectPlatformSpec
Aded more check to the assert helpers to improve LibFile coverage
Fixed problem with tempDir support when tempDir doesn't already exist
Creating temp dirs with varying permissions to trigger problem cases Also added simulated case for being unable to immediately clean-up the temp file
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.
Looks good to me. I do noticed a lot of final
where it doesn't makes sense to have them, e.g. static methods or in final classes.
Also, I would prefer to see the fake lib files to be generated by the test rather than committing them.
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.
nitpick: I suggest to make the test generate all these fake library files.
components/native-loader/src/main/java/datadog/nativeloader/NativeLoader.java
Show resolved
Hide resolved
} | ||
|
||
/** Loads a library associated with an associated component */ | ||
public final void load(String component, String libName) throws LibraryLoadException { |
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.
nitpick: some methods here and there are final
, but so is the class and its builder.
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.
Yeah, I have a very strong bias towards making things final to avoid confusing overrides. Admittedly, making the method final when the class is already final is redundant.
I still tend to make things final, so that if I change the class for some reason, I still have to make a conscious decision about the method.
components/native-loader/src/main/java/datadog/nativeloader/LibraryResolvers.java
Show resolved
Hide resolved
components/native-loader/src/main/java/datadog/nativeloader/PathLocator.java
Show resolved
Hide resolved
components/native-loader/src/test/java/datadog/nativeloader/FlatResolverTest.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
public static final PathLocator fromLibPathString(String javaLibPath) { | ||
return fromLibDirs(Pattern.compile("\\:").split(javaLibPath)); |
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: I believe the escape of :
isn't necessary here, while this code should happen I kinda prefer to have the Pattern.compile
stored in a constant.
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.
Typically, I would agree, but I'm expecting this to be used infrequently (usually once or less). And for that reason, I don't want to have a permanent reference to the compiled Pattern.
components/native-loader/src/test/java/datadog/nativeloader/IntrospectPlatformSpecTest.java
Outdated
Show resolved
Hide resolved
public static final boolean WITH_OMIT_COMP_FALLBACK = true; | ||
public static final boolean WITHOUT_OMIT_COMP_FALLBACK = false; |
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: Use an enum instead.
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.
Typically, I'd prefer not have extra immortal objects in memory. In this case, it is a test class, so maybe.
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 thought about that but it's for tests, so believe this okish :)
components/native-loader/src/test/java/datadog/nativeloader/NativeLoaderTest.java
Outdated
Show resolved
Hide resolved
…atDirLibraryResolver.java Co-authored-by: Brice Dutheil <brice.dutheil@gmail.com>
…ava into dougqh/library-loader
Added testFailOnExceptions helper to CapturingPathLocator Updated nested & flat resolvers tests to use new helper method Renamed test classes to match their corresponding classes
Added a 2-arg version of PathUtils.concatPath Using new concatPath version in LibDirBasedPathLocator Added some header doc to both versions of concatPath
Introducing NativeLoader API intended to standardize how native libraries are loaded in dd-trace-java
NativeLoader allows for using different file layout and file resolution strategies
What Does This Do
Introduces a new API, subsequent pull requests will use this new API to incorporate libdatadog.
Also intend to update profiling and ASM to use this new API
Motivation
This API will help standardize how native libraries are loaded in dd-java-agent.
The API also provides the means to switch between loading from the file system and ClassLoaders seamlessly without updating all of the calling code.