Skip to content
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

Adds Graal hints for the shaded dependencies #4832

Merged
merged 8 commits into from Oct 18, 2022

Conversation

marcingrzejszczak
Copy link
Contributor

we've noticed that the shaded jctools do not work with Graal out of the box. When trying to run the native image we get the following exception

Caused by: java.lang.RuntimeException: java.lang.NoSuchFieldException: producerIndex
        at io.opentelemetry.internal.shaded.jctools.util.UnsafeAccess.fieldOffset(UnsafeAccess.java:111) ~[na:na]
        at io.opentelemetry.internal.shaded.jctools.queues.MpscArrayQueueProducerIndexField.<clinit>(MpscArrayQueue.java:48) ~[demo-aot-native:1.18.0]
        ... 203 common frames omitted
Caused by: java.lang.NoSuchFieldException: producerIndex
        at java.lang.Class.getDeclaredField(DynamicHub.java:968) ~[demo-aot-native:na]
        at io.opentelemetry.internal.shaded.jctools.util.UnsafeAccess.fieldOffset(UnsafeAccess.java:107) ~[na:na]
        ... 204 common frames omitted

That's because the shaded dependencies are missing the META-INF/native-image/reflect-config.json. I followed the path taken by rsocket (rsocket/rsocket-java@f15c14a).

cc @sdeleuze

@marcingrzejszczak marcingrzejszczak requested a review from a team as a code owner October 10, 2022 10:30
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 10, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: marcingrzejszczak / name: Marcin Grzejszczak (7f50996)

@sdeleuze
Copy link

sdeleuze commented Oct 10, 2022

@marcingrzejszczak Could you please update the PR and test it with the file moved to sdk/trace-shaded-deps/src/main/resources/META-INF/native-image/io.opentelemetry/opentelemetry-sdk-trace/reflect-config.json in order to follow GraalVM guidelines?

Copy link
Member

@trask trask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx @marcingrzejszczak! is there any strategy for automating this / keeping it up-to-date over time? if there's no way to automate, it would be great if you could add a test for it, so that we don't accidentally introduce regressions later on

@sdeleuze
Copy link

It is possible to run JUnit test on native via https://github.com/graalvm/native-build-tools. You can just reuse existing tests or write a simple integration one that runs on native. It requires a JVM distribution with native-image like GraalVM CE (available on GitHub actions) or Liberica NIK.

@marcingrzejszczak
Copy link
Contributor Author

I've added tests and a Github Action build

@marcingrzejszczak
Copy link
Contributor Author

I've added tests and an additional github action check for graal

@trask
Copy link
Member

trask commented Oct 11, 2022

@marcingrzejszczak can you try ./gradlew :integration-tests:graal:spotlessApply to fix CI spotless failure?

@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Base: 90.79% // Head: 90.83% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (0910f7a) compared to base (d1a17d7).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #4832      +/-   ##
============================================
+ Coverage     90.79%   90.83%   +0.04%     
- Complexity     4844     4851       +7     
============================================
  Files           555      555              
  Lines         14438    14451      +13     
  Branches       1405     1409       +4     
============================================
+ Hits          13109    13127      +18     
+ Misses          910      906       -4     
+ Partials        419      418       -1     
Impacted Files Coverage Δ
...telemetry/sdk/trace/export/BatchSpanProcessor.java 93.10% <0.00%> (+0.68%) ⬆️
...sdk/autoconfigure/LoggerProviderConfiguration.java 95.00% <0.00%> (+2.40%) ⬆️
...dk/logs/export/BatchLogRecordProcessorBuilder.java 96.96% <0.00%> (+12.12%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@marcingrzejszczak
Copy link
Contributor Author

Is there anything else to be done here to merge this?

Copy link
Member

@jack-berg jack-berg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of small comments, but looks good. Thanks!

integration-tests/graal/build.gradle.kts Outdated Show resolved Hide resolved
integration-tests/graal/build.gradle.kts Outdated Show resolved Hide resolved
integration-tests/graal/build.gradle.kts Outdated Show resolved Hide resolved
integration-tests/graal/build.gradle.kts Outdated Show resolved Hide resolved
integration-tests/graal/build.gradle.kts Show resolved Hide resolved
@marcingrzejszczak
Copy link
Contributor Author

Changes got applied, thanks for the review!

@jack-berg jack-berg merged commit 9a1996c into open-telemetry:main Oct 18, 2022
@fniephaus
Copy link

we've noticed that the shaded jctools do not work with Graal out of the box

Does this mean that opentelemetry-java now works with GraalVM Native Image out of the box or is something still not working?

@marcingrzejszczak
Copy link
Contributor Author

Since this got merged that means that indeed it does work with Native. You can check my post on Spring.io where I demonstrate that - https://spring.io/blog/2022/10/12/observability-with-spring-boot-3#running-it-all-together-with-aot-support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants