fix: add runtime hints for trace #1990
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Pasting in error message for reference: |
|
A note for GraalVM for JDK 17 implementation (not applicable to GraalVM 22.3.2): The argument to enable RunReachabilityHandlersConcurrently feature is needed for this sample in GraalVM for JDK 17 (GraalVM 23): Without this argument, the tests fail at build time with the following error: |
mpeddada1
reviewed
Jul 19, 2023
Comment on lines
42
to
43
| TypeReference.of(com.google.protobuf.Value.class), | ||
| TypeReference.of(com.google.protobuf.Value.Builder.class)), |
There was a problem hiding this comment.
These two configurations may be removed after the static jsons are generated through googleapis/sdk-platform-java#1841. For now, we get the following error without these configurations:
Failures (1):
JUnit Jupiter:TraceSampleApplicationIntegrationTests:testTracesAreLoggedCorrectly()
MethodSource [className = 'com.example.TraceSampleApplicationIntegrationTests', methodName = 'testTracesAreLoggedCorrectly', methodParameterTypes = '']
=> java.lang.IllegalStateException: Generated message class "com.google.protobuf.Value$Builder" missing method "hasNullValue".
com.google.protobuf.GeneratedMessageV3.getMethodOrDie(GeneratedMessageV3.java:1998)
com.google.protobuf.GeneratedMessageV3.access$1000(GeneratedMessageV3.java:79)
com.google.protobuf.GeneratedMessageV3$FieldAccessorTable$SingularFieldAccessor$ReflectionInvoker.<init>(GeneratedMessageV3.java:2353)
com.google.protobuf.GeneratedMessageV3$FieldAccessorTable$SingularFieldAccessor.<init>(GeneratedMessageV3.java:2418)
com.google.protobuf.GeneratedMessageV3$FieldAccessorTable$SingularEnumFieldAccessor.<init>(GeneratedMessageV3.java:2893)
[...]
Caused by: java.lang.NoSuchMethodException: com.google.protobuf.Value$Builder.hasNullValue()
[email protected]/java.lang.Class.checkMethod(DynamicHub.java:1038)
[email protected]/java.lang.Class.getMethod(DynamicHub.java:1023)
com.google.protobuf.GeneratedMessageV3.getMethodOrDie(GeneratedMessageV3.java:1995)
[...]
There was a problem hiding this comment.
I don't see com.google.protobuf.Value as an entry in the Trace library's reflect-config.json. It does show up for some, though: https://github.com/search?q=repo%3Agoogleapis%2Fgoogle-cloud-java+com.google.protobuf.Value+path%3A**%2Freflect-config.json&type=code&p=1
- Do we need to adjust the generation logic to include more than messages defined in the protos, or is a non-trace library triggering the reflection lookup?
There was a problem hiding this comment.
Good point! Analyzing the stacktrace of the test shows that the use of com.google.protobuf.Value is coming from
This line of code eventually leads to GeneratedMessageV3#getMethodOrDie being called:
I've moved the configuration to the test/ directory.
There was a problem hiding this comment.
[Update] Tests are passing without these configurations after rebasing with origin/main.
mpeddada1
reviewed
Jul 21, 2023
Comment on lines
+26
to
+34
|
|
||
| @Override | ||
| public void registerHints(RuntimeHints hints, ClassLoader classLoader) { | ||
| hints.reflection().registerTypes( | ||
| Arrays.asList(TypeReference.of(Application.class)), | ||
| hint -> hint.withMembers( | ||
| MemberCategory.INVOKE_PUBLIC_METHODS)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Without this configuration, the sample test results in:
com.example.TraceSampleApplicationIntegrationTests > testTracesAreLoggedCorrectly() FAILED
Failures (1):
JUnit Jupiter:TraceSampleApplicationIntegrationTests:testTracesAreLoggedCorrectly()
MethodSource [className = 'com.example.TraceSampleApplicationIntegrationTests', methodName = 'testTracesAreLoggedCorrectly', methodParameterTypes = '']
=> com.oracle.svm.core.jdk.UnsupportedFeatureError: ThreadMXBean methods
org.graalvm.nativeimage.builder/com.oracle.svm.core.util.VMError.unsupportedFeature(VMError.java:89)
org.graalvm.nativeimage.builder/com.oracle.svm.core.jdk.management.SubstrateThreadMXBean.findDeadlockedThreads(SubstrateThreadMXBean.java:258)
org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:157)
org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:769)
com.example.TraceSampleApplicationIntegrationTests.testTracesAreLoggedCorrectly(TraceSampleApplicationIntegrationTests.java:191)
[email protected]/java.lang.reflect.Method.invoke(Method.java:568)
org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:727)
[...]
burkedavison
approved these changes
Jul 31, 2023
Comment on lines
42
to
43
| TypeReference.of(com.google.protobuf.Value.class), | ||
| TypeReference.of(com.google.protobuf.Value.Builder.class)), |
There was a problem hiding this comment.
I don't see com.google.protobuf.Value as an entry in the Trace library's reflect-config.json. It does show up for some, though: https://github.com/search?q=repo%3Agoogleapis%2Fgoogle-cloud-java+com.google.protobuf.Value+path%3A**%2Freflect-config.json&type=code&p=1
- Do we need to adjust the generation logic to include more than messages defined in the protos, or is a non-trace library triggering the reflection lookup?
|
Thanks for the review @burkedavison! Adding some initial findings here. The dependency tree of the sample shows that the protobuf-java dependency which contains the Digging deeper to see if the stacktrace gives us more clues on how this class is being invoked. |
|
Subdirectories for aot.factories don't get recognized. The trace sample fails with: Using ImportRuntimeHints annotation instead to resolve the failure mentioned in #1990 (comment) |
|
Kudos, SonarCloud Quality Gate passed! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Builds on top of CI setup in #1933. Will rebase once its merged in.This pr:
TraceRuntimeHintsin trace autoconfig module and unit test for it. This allows native builds.SampleRuntimeHintsthis enables pubsub trace portion of the sample to run after Native Build. not exactly sure why this is needed.TestRuntimeHintsto allow logging portion of the test setup run as expect. (same as fix: add runtime hints for logging #1933)CI test should run sample integration test.
To test run manually:
mvn clean package -Pnative-sample-config -Pnative, then run application with./target/spring-cloud-gcp-trace-sample. Then follow sample README to test functionality.mvn -PnativeTest clean test -Pnative-sample-configexpect to get:
UPDATE: start failing on ci and local after rebase. Need investigate.