Issue: 289 - Adding GraalVM native-image support. #311
Conversation
Codecov Report
@@ Coverage Diff @@
## master #311 +/- ##
============================================
- Coverage 58.92% 58.90% -0.02%
+ Complexity 1206 1205 -1
============================================
Files 131 131
Lines 5066 5066
Branches 593 593
============================================
- Hits 2985 2984 -1
Misses 1806 1806
- Partials 275 276 +1
Continue to review full report at Codecov.
|
| { | ||
| "resources" : { | ||
| "includes" : [ { | ||
| "pattern" : "\\Qcom/amazonaws/xray/interceptors/DefaultOperationParameterWhitelist.json\\E" |
There was a problem hiding this comment.
What do the Q and E mean? I assume just delimiters for beginning and ending the expression, just strange the usual regex characters aren't used.
There was a problem hiding this comment.
The pattern attribute is a Java regexp that matches resource(s) to be included in the image from https://www.graalvm.org/reference-manual/native-image/Resources/
I imagine this was created using the tracing agent so might have an auto generated feel :)
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.amazonaws.xray.strategy.sampling.reservoir.Reservoir", |
There was a problem hiding this comment.
I have the same question here that @anuraaga had in the original issue, to quote him:
My understanding is graal would only need these for code that is accessed through reflection. I understand many of the classes are because Jackson accesses the fields reflectively, but there seem to be some like "name": "com.amazonaws.xray.strategy.sampling.reservoir.Reservoir", and friends that don't. Do you know if these should be included?
There was a problem hiding this comment.
Just check I understand the question. Do you believe that this is never accessed via reflection? and isn't required?
There was a problem hiding this comment.
Yes that is my understanding, but I could be wrong. I guess the author of these configs probably listed all these classes based on experimentation. If that's the case, then it's fine, just. interesting that some part of this logic (probably via Jackson) ends up accessing this class reflectively.
There was a problem hiding this comment.
I tested the config with this project https://github.com/aws-samples/serverless-graalvm-demo/tree/aws-xray-support and it was successful. That doesn't mean it isn't a false positive. I'll test it tomorrow.
There was a problem hiding this comment.
Sounds good, if removing them causes your test to fail then it's fine to leave as-is
There was a problem hiding this comment.
Test failed without.
| "allDeclaredConstructors": true | ||
| }, | ||
| { | ||
| "name": "com.amazonaws.xray.strategy.sampling.reservoir.Reservoir$MaxFunction", |
There was a problem hiding this comment.
Does it matter that these static nested classes are package-private?
There was a problem hiding this comment.
No, they have to be added individually.
Issue #, if available:
#289
Description of changes:
Adding GraalVM native-image config
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.