-
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: fa0f8a8 | 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 0 performance improvements and 0 performance regressions! Performance is the same for 58 metrics, 7 unstable metrics. Startup time reports for petclinicgantt
title petclinic - global startup overhead: candidate=1.54.0-SNAPSHOT~fa0f8a8838, baseline=1.55.0-SNAPSHOT~1772b5b0ee
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.02 s) : 0, 1020119
Total [baseline] (10.738 s) : 0, 10737915
Agent [candidate] (1.02 s) : 0, 1020499
Total [candidate] (10.678 s) : 0, 10677865
section appsec
Agent [baseline] (1.201 s) : 0, 1200554
Total [baseline] (11.027 s) : 0, 11027044
Agent [candidate] (1.192 s) : 0, 1191662
Total [candidate] (11.058 s) : 0, 11058119
section iast
Agent [baseline] (1.15 s) : 0, 1149658
Total [baseline] (10.929 s) : 0, 10929308
Agent [candidate] (1.151 s) : 0, 1150932
Total [candidate] (10.999 s) : 0, 10999339
section profiling
Agent [baseline] (1.162 s) : 0, 1162481
Total [baseline] (11.069 s) : 0, 11069037
Agent [candidate] (1.17 s) : 0, 1169952
Total [candidate] (11.087 s) : 0, 11087449
gantt
title petclinic - break down per module: candidate=1.54.0-SNAPSHOT~fa0f8a8838, baseline=1.55.0-SNAPSHOT~1772b5b0ee
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.464 ms) : 0, 1464
crashtracking [candidate] (1.459 ms) : 0, 1459
BytebuddyAgent [baseline] (695.36 ms) : 0, 695360
BytebuddyAgent [candidate] (695.952 ms) : 0, 695952
GlobalTracer [baseline] (241.583 ms) : 0, 241583
GlobalTracer [candidate] (241.744 ms) : 0, 241744
AppSec [baseline] (32.975 ms) : 0, 32975
AppSec [candidate] (32.935 ms) : 0, 32935
Debugger [baseline] (6.408 ms) : 0, 6408
Debugger [candidate] (6.423 ms) : 0, 6423
Remote Config [baseline] (689.259 µs) : 0, 689
Remote Config [candidate] (690.588 µs) : 0, 691
Telemetry [baseline] (9.347 ms) : 0, 9347
Telemetry [candidate] (9.381 ms) : 0, 9381
Flare Poller [baseline] (11.062 ms) : 0, 11062
Flare Poller [candidate] (10.738 ms) : 0, 10738
section appsec
crashtracking [baseline] (1.471 ms) : 0, 1471
crashtracking [candidate] (1.449 ms) : 0, 1449
BytebuddyAgent [baseline] (722.527 ms) : 0, 722527
BytebuddyAgent [candidate] (716.253 ms) : 0, 716253
GlobalTracer [baseline] (235.821 ms) : 0, 235821
GlobalTracer [candidate] (234.019 ms) : 0, 234019
IAST [baseline] (25.091 ms) : 0, 25091
IAST [candidate] (24.829 ms) : 0, 24829
AppSec [baseline] (175.246 ms) : 0, 175246
AppSec [candidate] (174.897 ms) : 0, 174897
Debugger [baseline] (6.115 ms) : 0, 6115
Debugger [candidate] (6.117 ms) : 0, 6117
Remote Config [baseline] (639.91 µs) : 0, 640
Remote Config [candidate] (648.971 µs) : 0, 649
Telemetry [baseline] (8.471 ms) : 0, 8471
Telemetry [candidate] (8.483 ms) : 0, 8483
Flare Poller [baseline] (3.964 ms) : 0, 3964
Flare Poller [candidate] (3.962 ms) : 0, 3962
section iast
crashtracking [baseline] (1.458 ms) : 0, 1458
crashtracking [candidate] (1.466 ms) : 0, 1466
BytebuddyAgent [baseline] (813.93 ms) : 0, 813930
BytebuddyAgent [candidate] (815.323 ms) : 0, 815323
GlobalTracer [baseline] (231.485 ms) : 0, 231485
GlobalTracer [candidate] (231.914 ms) : 0, 231914
IAST [baseline] (26.545 ms) : 0, 26545
IAST [candidate] (26.503 ms) : 0, 26503
AppSec [baseline] (35.428 ms) : 0, 35428
AppSec [candidate] (35.124 ms) : 0, 35124
Debugger [baseline] (6.124 ms) : 0, 6124
Debugger [candidate] (6.091 ms) : 0, 6091
Remote Config [baseline] (608.151 µs) : 0, 608
Remote Config [candidate] (604.621 µs) : 0, 605
Telemetry [baseline] (8.722 ms) : 0, 8722
Telemetry [candidate] (8.594 ms) : 0, 8594
Flare Poller [baseline] (4.149 ms) : 0, 4149
Flare Poller [candidate] (4.105 ms) : 0, 4105
section profiling
crashtracking [baseline] (1.443 ms) : 0, 1443
crashtracking [candidate] (1.445 ms) : 0, 1445
BytebuddyAgent [baseline] (720.909 ms) : 0, 720909
BytebuddyAgent [candidate] (726.195 ms) : 0, 726195
GlobalTracer [baseline] (217.431 ms) : 0, 217431
GlobalTracer [candidate] (218.686 ms) : 0, 218686
AppSec [baseline] (33.205 ms) : 0, 33205
AppSec [candidate] (33.582 ms) : 0, 33582
Debugger [baseline] (7.261 ms) : 0, 7261
Debugger [candidate] (6.56 ms) : 0, 6560
Remote Config [baseline] (698.226 µs) : 0, 698
Remote Config [candidate] (709.931 µs) : 0, 710
Telemetry [baseline] (15.851 ms) : 0, 15851
Telemetry [candidate] (15.969 ms) : 0, 15969
Flare Poller [baseline] (4.249 ms) : 0, 4249
Flare Poller [candidate] (5.0 ms) : 0, 5000
ProfilingAgent [baseline] (108.592 ms) : 0, 108592
ProfilingAgent [candidate] (108.148 ms) : 0, 108148
Profiling [baseline] (109.587 ms) : 0, 109587
Profiling [candidate] (109.693 ms) : 0, 109693
Startup time reports for insecure-bankgantt
title insecure-bank - global startup overhead: candidate=1.54.0-SNAPSHOT~fa0f8a8838, baseline=1.55.0-SNAPSHOT~1772b5b0ee
dateFormat X
axisFormat %s
section tracing
Agent [baseline] (1.02 s) : 0, 1020390
Total [baseline] (8.668 s) : 0, 8667974
Agent [candidate] (1.025 s) : 0, 1024517
Total [candidate] (8.673 s) : 0, 8672695
section iast
Agent [baseline] (1.173 s) : 0, 1172649
Total [baseline] (9.272 s) : 0, 9271785
Agent [candidate] (1.152 s) : 0, 1151637
Total [candidate] (9.262 s) : 0, 9261736
gantt
title insecure-bank - break down per module: candidate=1.54.0-SNAPSHOT~fa0f8a8838, baseline=1.55.0-SNAPSHOT~1772b5b0ee
dateFormat X
axisFormat %s
section tracing
crashtracking [baseline] (1.462 ms) : 0, 1462
crashtracking [candidate] (1.471 ms) : 0, 1471
BytebuddyAgent [baseline] (694.824 ms) : 0, 694824
BytebuddyAgent [candidate] (698.835 ms) : 0, 698835
GlobalTracer [baseline] (242.814 ms) : 0, 242814
GlobalTracer [candidate] (242.932 ms) : 0, 242932
AppSec [baseline] (32.969 ms) : 0, 32969
AppSec [candidate] (33.288 ms) : 0, 33288
Debugger [baseline] (6.427 ms) : 0, 6427
Debugger [candidate] (6.475 ms) : 0, 6475
Remote Config [baseline] (696.611 µs) : 0, 697
Remote Config [candidate] (696.861 µs) : 0, 697
Telemetry [baseline] (9.331 ms) : 0, 9331
Telemetry [candidate] (9.368 ms) : 0, 9368
Flare Poller [baseline] (10.856 ms) : 0, 10856
Flare Poller [candidate] (10.266 ms) : 0, 10266
section iast
crashtracking [baseline] (1.488 ms) : 0, 1488
crashtracking [candidate] (1.455 ms) : 0, 1455
BytebuddyAgent [baseline] (832.855 ms) : 0, 832855
BytebuddyAgent [candidate] (815.86 ms) : 0, 815860
GlobalTracer [baseline] (234.334 ms) : 0, 234334
GlobalTracer [candidate] (231.735 ms) : 0, 231735
IAST [baseline] (27.067 ms) : 0, 27067
IAST [candidate] (27.434 ms) : 0, 27434
AppSec [baseline] (35.805 ms) : 0, 35805
AppSec [candidate] (34.445 ms) : 0, 34445
Debugger [baseline] (6.159 ms) : 0, 6159
Debugger [candidate] (6.15 ms) : 0, 6150
Remote Config [baseline] (622.067 µs) : 0, 622
Remote Config [candidate] (600.928 µs) : 0, 601
Telemetry [baseline] (8.727 ms) : 0, 8727
Telemetry [candidate] (8.655 ms) : 0, 8655
Flare Poller [baseline] (4.098 ms) : 0, 4098
Flare Poller [candidate] (4.181 ms) : 0, 4181
LoadParameters
See matching parameters
SummaryFound 3 performance improvements and 0 performance regressions! Performance is the same for 9 metrics, 12 unstable metrics.
Request duration reports for petclinicgantt
title petclinic - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~fa0f8a8838, baseline=1.55.0-SNAPSHOT~1772b5b0ee
dateFormat X
axisFormat %s
section baseline
no_agent (37.319 ms) : 37023, 37615
. : milestone, 37319,
appsec (49.314 ms) : 48881, 49747
. : milestone, 49314,
code_origins (43.535 ms) : 43163, 43907
. : milestone, 43535,
iast (46.124 ms) : 45747, 46501
. : milestone, 46124,
profiling (47.359 ms) : 46936, 47782
. : milestone, 47359,
tracing (45.139 ms) : 44757, 45522
. : milestone, 45139,
section candidate
no_agent (37.877 ms) : 37575, 38180
. : milestone, 37877,
appsec (47.824 ms) : 47379, 48269
. : milestone, 47824,
code_origins (44.441 ms) : 44063, 44819
. : milestone, 44441,
iast (45.84 ms) : 45451, 46229
. : milestone, 45840,
profiling (47.745 ms) : 47254, 48236
. : milestone, 47745,
tracing (43.591 ms) : 43212, 43970
. : milestone, 43591,
Request duration reports for insecure-bankgantt
title insecure-bank - request duration [CI 0.99] : candidate=1.54.0-SNAPSHOT~fa0f8a8838, baseline=1.55.0-SNAPSHOT~1772b5b0ee
dateFormat X
axisFormat %s
section baseline
no_agent (4.409 ms) : 4358, 4461
. : milestone, 4409,
iast (10.278 ms) : 10105, 10451
. : milestone, 10278,
iast_FULL (14.337 ms) : 14055, 14619
. : milestone, 14337,
iast_GLOBAL (10.619 ms) : 10424, 10814
. : milestone, 10619,
profiling (8.799 ms) : 8660, 8937
. : milestone, 8799,
tracing (7.671 ms) : 7548, 7794
. : milestone, 7671,
section candidate
no_agent (4.324 ms) : 4273, 4376
. : milestone, 4324,
iast (9.433 ms) : 9279, 9587
. : milestone, 9433,
iast_FULL (13.953 ms) : 13682, 14223
. : milestone, 13953,
iast_GLOBAL (10.403 ms) : 10217, 10589
. : milestone, 10403,
profiling (8.729 ms) : 8590, 8867
. : milestone, 8729,
tracing (7.467 ms) : 7361, 7574
. : milestone, 7467,
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 biojavagantt
title biojava - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~fa0f8a8838, baseline=1.55.0-SNAPSHOT~1772b5b0ee
dateFormat X
axisFormat %s
section baseline
no_agent (14.92 s) : 14920000, 14920000
. : milestone, 14920000,
appsec (15.165 s) : 15165000, 15165000
. : milestone, 15165000,
iast (18.642 s) : 18642000, 18642000
. : milestone, 18642000,
iast_GLOBAL (17.745 s) : 17745000, 17745000
. : milestone, 17745000,
profiling (14.83 s) : 14830000, 14830000
. : milestone, 14830000,
tracing (15.049 s) : 15049000, 15049000
. : milestone, 15049000,
section candidate
no_agent (15.473 s) : 15473000, 15473000
. : milestone, 15473000,
appsec (14.96 s) : 14960000, 14960000
. : milestone, 14960000,
iast (18.441 s) : 18441000, 18441000
. : milestone, 18441000,
iast_GLOBAL (18.015 s) : 18015000, 18015000
. : milestone, 18015000,
profiling (15.432 s) : 15432000, 15432000
. : milestone, 15432000,
tracing (15.155 s) : 15155000, 15155000
. : milestone, 15155000,
Execution time for tomcatgantt
title tomcat - execution time [CI 0.99] : candidate=1.54.0-SNAPSHOT~fa0f8a8838, baseline=1.55.0-SNAPSHOT~1772b5b0ee
dateFormat X
axisFormat %s
section baseline
no_agent (1.47 ms) : 1458, 1481
. : milestone, 1470,
appsec (3.717 ms) : 3500, 3933
. : milestone, 3717,
iast (2.204 ms) : 2141, 2267
. : milestone, 2204,
iast_GLOBAL (2.248 ms) : 2185, 2312
. : milestone, 2248,
profiling (2.042 ms) : 1991, 2093
. : milestone, 2042,
tracing (2.036 ms) : 1987, 2085
. : milestone, 2036,
section candidate
no_agent (1.473 ms) : 1462, 1484
. : milestone, 1473,
appsec (3.701 ms) : 3483, 3919
. : milestone, 3701,
iast (2.2 ms) : 2137, 2262
. : milestone, 2200,
iast_GLOBAL (2.251 ms) : 2187, 2315
. : milestone, 2251,
profiling (2.461 ms) : 2235, 2687
. : milestone, 2461,
tracing (2.025 ms) : 1976, 2074
. : milestone, 2025,
|
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 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.
components/native-loader/src/main/java/datadog/nativeloader/NativeLoader.java
Show resolved
Hide resolved
components/native-loader/src/main/java/datadog/nativeloader/NativeLoader.java
Outdated
Show resolved
Hide resolved
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
components/native-loader/src/main/java/datadog/nativeloader/PathLocators.java
Show resolved
Hide resolved
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
Switched to creating test jar from test-data directory Added various utility functions to make tests easier to read
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.