-
Notifications
You must be signed in to change notification settings - Fork 913
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
Spring boot runtime metrics native reduced #13173
Spring boot runtime metrics native reduced #13173
Conversation
@jeanbisutti here's a version that supports a subset of metrics for native mode - I'd suggest we start here and add more coverage for native mode later |
0545a83
to
7c68b99
Compare
@zeitlinger Could you please elaborate a bit what this PR is doing? |
sure @jeanbisutti Initially, I tried to add all runtime metric features to native image - but it got too difficult.
|
In the tests, JFR is now not enable at the the build time: 6850c88#diff-75989ca3659128c1bc6aea21d1129a7a704b98959fad35345d5c3c216c53c91cL71 Is it related to the native build failure mentioned in #13078 (comment)? If there is JFR-related code, even programmatically disabled, could you please renable JFR at build time in the tests to check potential issues at the build time? Ideally, if the JFR code can't be used in the native image, I would prefer to not include it at the build time (see #13078 (comment)) to avoid side effects at the build time (and help limit the native image size). |
yes
JFR is enabled differently based on the graalvm version.
I don't think it's enabled - can you point me to the relevant change? |
and/or new added code causing build time issues
It's not what I have said. There is JFR-related code programatically disabled at runtime ( Line 139 in 7c68b99
|
@jeanbisutti got that working now 😄 |
...ure/internal/instrumentation/runtimemetrics/RuntimeMetricsBeanRegistrationExcludeFilter.java
Outdated
Show resolved
Hide resolved
...ure/internal/instrumentation/runtimemetrics/RuntimeMetricsBeanRegistrationExcludeFilter.java
Show resolved
Hide resolved
...rary/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/java8/RuntimeMetrics.java
Outdated
Show resolved
Hide resolved
...pring/autoconfigure/internal/instrumentation/runtimemetrics/Java8RuntimeMetricsProvider.java
Outdated
Show resolved
Hide resolved
...g/autoconfigure/internal/instrumentation/runtimemetrics/RuntimeMetricsAutoConfiguration.java
Outdated
Show resolved
Hide resolved
@jeanbisutti please check again |
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.
@zeitlinger After a lot of discussions, it looks generally good to me.
I have approved with some comments.
...java/io/opentelemetry/instrumentation/runtimemetrics/java17/Java17RuntimeMetricsBuilder.java
Outdated
Show resolved
Hide resolved
...va8/library/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/java8/Threads.java
Outdated
Show resolved
Hide resolved
...io/opentelemetry/instrumentation/runtimemetrics/java8/internal/JmxRuntimeMetricsFactory.java
Outdated
Show resolved
Hide resolved
...ure/internal/instrumentation/runtimemetrics/RuntimeMetricsBeanRegistrationExcludeFilter.java
Outdated
Show resolved
Hide resolved
...c/main/java/io/opentelemetry/instrumentation/runtimemetrics/java17/Java17RuntimeMetrics.java
Outdated
Show resolved
Hide resolved
…Spring3/io/opentelemetry/instrumentation/spring/autoconfigure/internal/instrumentation/runtimemetrics/RuntimeMetricsBeanRegistrationExcludeFilter.java Co-authored-by: Jean Bisutti <[email protected]>
…ary/src/main/java/io/opentelemetry/instrumentation/runtimemetrics/java8/Threads.java Co-authored-by: Jean Bisutti <[email protected]>
88d21b7
to
53a3e6a
Compare
trimmed down version of #13078