fix: add runtime hints for logging #1933

zhumin8 · 2023-06-07T21:11:10Z

~~Need rebase after setups in #1931 are merged in.~~

This pr adds:

runtime hints for springframework aot to allow Logging module work with Native build.
a relevant unit test
runtime hints needed for sample integration test. (this is test setup specific and is not needed to run sample application).

CI for native is setup to run on sample integration tests that:

has it.xx property added to systemPropertyVariables
module added to native-sample-config profile.

Modification to spring-cloud-gcp-samples/pom.xml so that:

by default, all modules are included.
To run native test only for selected modules, use --define notAllModules=true in mvn cmd.

This is needed because native test would fail on no test configuration found.

To verify locally:

Run the sample manually:
- Follow guide in README, instead of mvn spring-boot:run, package with mvn clean package -Pnative-sample-config -Pnative
- Run executable ./target/spring-cloud-gcp-logging-sample
- Follow guide to issue a request and observe log lines from cloud console.
- Log entry written by sample:

{
insertId: "1c08xcxf1v07m9"
jsonPayload: {
message: "This line was also written to the log with the same Trace ID."
}
labels: {3}
logName: "projects/[project-id]/logs/spring.log"
receiveTimestamp: "2023-06-06T14:15:08.236653291Z"
resource: {2}
severity: "INFO"
timestamp: "2023-06-06T14:15:08.122Z"
trace: "projects/[project-id]/traces/trace-id-to-show-1234"
}

Run sample test:
- mvn -PnativeTest clean test -Pnative-sample-config
- native test are run at the end of the build.

com.example.LoggingSampleApplicationIntegrationTests > testLogRecordedInStackDriver() SUCCESSFUL


Test run finished after 12975 ms
[         3 containers found      ]
[         0 containers skipped    ]
[         3 containers started    ]
[         0 containers aborted    ]
[         3 containers successful ]
[         0 containers failed     ]
[         1 tests found           ]
[         0 tests skipped         ]
[         1 tests started         ]
[         0 tests aborted         ]
[         1 tests successful      ]
[         0 tests failed          ]

zhumin8 · 2023-06-28T20:21:35Z

force-pushed to rebase on main.

…property when doing native test.

…oot-starter-parent.

.github/workflows/NativeTests.yaml

burkedavison · 2023-07-05T19:00:29Z

.github/workflows/NativeTests.yaml

+      - name: Install pubsub-emulator
+        if: ${{ matrix.it == 'pubsub-emulator' }}
+        run: |
+          gcloud components install pubsub-emulator beta && \
+            gcloud components update


I think matrix.it is always null. Can this step be removed?

mpeddada1 · 2023-07-05T19:26:25Z

...cloud-gcp-logging/src/main/java/com/google/cloud/spring/logging/aot/LoggingRuntimeHints.java

+public class LoggingRuntimeHints implements RuntimeHintsRegistrar {
+  @Override
+  public void registerHints(RuntimeHints hints, ClassLoader classLoader) {
+    hints
+        .reflection()
+        .registerTypes(
+            Arrays.asList(
+                TypeReference.of(LoggingAppender.class),
+                TypeReference.of(TraceIdLoggingEnhancer.class),
+                TypeReference.of(StackdriverJsonLayout.class)),
+            hint ->
+                hint.withMembers(
+                    MemberCategory.INVOKE_DECLARED_CONSTRUCTORS,
+                    MemberCategory.INVOKE_PUBLIC_METHODS));
+  }
+}


Out of curiosity, do we notice any advantages with using the RuntimeHintsRegistrar class over using static reflection configurations?
Mostly in terms of choosing the static jsons vs a dynamic way of registering classes for reflection, I'm wondering if we will run into any breaking changes when we upgrade our Spring stack that introduces some changes to it's class definition.

Good question! I went this way because it's documented and to be consistent to spring conventions, I notice spring-boot and spring-data projects are using this RuntimeHintsRegistar (e.g. logging autoconfig)
Some other benefit include: this allows hints be tested; It's easy use spring concepts when registering hints (for example in vision, this is for the test resources, but you can easily refer to spring resources when registering.)

Thanks for the context! Considering this resolved to unblock this PR. Discussion has been moved offline.

sonarcloud · 2023-07-05T20:14:36Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

mpeddada1 · 2023-07-05T22:00:00Z

...cloud-gcp-logging/src/main/java/com/google/cloud/spring/logging/aot/LoggingRuntimeHints.java

+public class LoggingRuntimeHints implements RuntimeHintsRegistrar {
+  @Override
+  public void registerHints(RuntimeHints hints, ClassLoader classLoader) {
+    hints
+        .reflection()
+        .registerTypes(
+            Arrays.asList(
+                TypeReference.of(LoggingAppender.class),
+                TypeReference.of(TraceIdLoggingEnhancer.class),
+                TypeReference.of(StackdriverJsonLayout.class)),
+            hint ->
+                hint.withMembers(
+                    MemberCategory.INVOKE_DECLARED_CONSTRUCTORS,
+                    MemberCategory.INVOKE_PUBLIC_METHODS));
+  }
+}


Thanks for the context! Considering this resolved to unblock this PR. Discussion has been moved offline.

zhumin8 mentioned this pull request Jun 8, 2023

chore: adding profiles for native compile testings #1931

Merged

zhumin8 force-pushed the logging-hints branch from 44e7dcd to 2b7eb8a Compare June 12, 2023 13:52

zhumin8 force-pushed the logging-hints branch 2 times, most recently from 1119f05 to 77e831c Compare June 27, 2023 15:18

zhumin8 marked this pull request as ready for review June 27, 2023 16:01

zhumin8 requested a review from a team as a code owner June 27, 2023 16:01

zhumin8 requested review from burkedavison and mpeddada1 June 27, 2023 16:01

zhumin8 force-pushed the logging-hints branch from 77e831c to 0ee92fa Compare June 28, 2023 20:20

zhumin8 added 16 commits June 28, 2023 21:12

chore: add hints so trace id is present in log.

56e591a

chore: remove unused hints and refactor class name location.

32b511f

chore: remove more unused hints, lint, add hint test.

b9e8a13

chore:add runtime hints for sample test.

0811a6c

chore: move full module list to a default profile, so can disable by …

7d82107

…property when doing native test.

chore: remove native-maven-plugin version. it inherites from spring-b…

6a2fab8

…oot-starter-parent.

chore: try to setup action ci for sample tests.

d1b892b

fix: github action test names.

dbdd6ca

fix: mvnw location.

335a835

fix: use notAllModules property to limit modules for this test.

a337dd6

fix: github action commands.

27ecea6

fix: add missing working dir.

5e4ba54

chore: update ci project to separate from it tests.

679feb6

chore:cleanup github action steps.

1629f72

remove redundant test.

9ae8db6

fix: ci test title.

a6257cd

zhumin8 force-pushed the logging-hints branch from 0ee92fa to a6257cd Compare June 29, 2023 01:12

This was referenced Jun 29, 2023

fix: add runtime hints for trace #1990

Merged

fix: add runtime hints for vision #1991

Merged

burkedavison reviewed Jul 5, 2023

View reviewed changes

.github/workflows/NativeTests.yaml Show resolved Hide resolved

burkedavison reviewed Jul 5, 2023

View reviewed changes

mpeddada1 reviewed Jul 5, 2023

View reviewed changes

zhumin8 mentioned this pull request Jul 5, 2023

fix: add runtime hints for storage #2001

Merged

chore: remove Install pubsub-emulator from ci.

27f7914

burkedavison approved these changes Jul 5, 2023

View reviewed changes

mpeddada1 approved these changes Jul 5, 2023

View reviewed changes

zhumin8 merged commit 21903af into main Jul 6, 2023
29 checks passed

zhumin8 deleted the logging-hints branch July 6, 2023 17:22

release-please bot mentioned this pull request Jul 6, 2023

chore(main): release 4.6.0 #2005

Merged

release-please bot mentioned this pull request Dec 18, 2023

chore(main): release 5.0.1 #2467

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add runtime hints for logging #1933

fix: add runtime hints for logging #1933

zhumin8 commented Jun 7, 2023 •

edited

zhumin8 commented Jun 28, 2023

burkedavison Jul 5, 2023

zhumin8 Jul 5, 2023

mpeddada1 Jul 5, 2023

zhumin8 Jul 5, 2023

mpeddada1 Jul 5, 2023

sonarcloud bot commented Jul 5, 2023

mpeddada1 Jul 5, 2023

fix: add runtime hints for logging #1933

fix: add runtime hints for logging #1933

Conversation

zhumin8 commented Jun 7, 2023 • edited

CI for native is setup to run on sample integration tests that:

To verify locally:

zhumin8 commented Jun 28, 2023

burkedavison Jul 5, 2023

Choose a reason for hiding this comment

zhumin8 Jul 5, 2023

Choose a reason for hiding this comment

mpeddada1 Jul 5, 2023

Choose a reason for hiding this comment

zhumin8 Jul 5, 2023

Choose a reason for hiding this comment

mpeddada1 Jul 5, 2023

Choose a reason for hiding this comment

sonarcloud bot commented Jul 5, 2023

mpeddada1 Jul 5, 2023

Choose a reason for hiding this comment

zhumin8 commented Jun 7, 2023 •

edited