-
Notifications
You must be signed in to change notification settings - Fork 883
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 #13078
base: main
Are you sure you want to change the base?
Spring boot runtime metrics #13078
Conversation
@jeanbisutti can you help me with the native failures:
|
@zeitlinger About 1., it seems an awaitility issue. Does the problem only appear with the new changes? Perhaps it may be worth to do something like Line 125 in edbd9aa
About 2., About 3., it seems related to Tomcat searching memory leaks. With the full log we could know if it really comes from Tomcat. It does not seem possible to stop the
Native tests of this PR are failing during the native compilation step:
I would try to focus on the JMX or JFR metrics for a GraalVM native execution. GraalVM supports some JFR events, but not all of them. So, not sure that all the JFR metrics can work today in the native mode. |
I haven't seen that log before. |
we also have |
c0de41a
to
b3c0f10
Compare
@jeanbisutti turned out that all prior errors were just a side effect of a jmx issue which is resolved now can you take a look again? |
@@ -209,6 +209,11 @@ void shouldSendTelemetry() { | |||
OtelSpringStarterSmokeTestController.METER_SCOPE_NAME, | |||
OtelSpringStarterSmokeTestController.TEST_HISTOGRAM, | |||
AbstractIterableAssert::isNotEmpty); | |||
// runtime metrics | |||
testing.waitAndAssertMetrics( |
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.
It seems to verify a thread-based runtime metric.
It may be worth to also verify that JMX-based and JFR-based runtime metrics work with the OpenTelemetry starter in both non-native and native modes.
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.
added @jeanbisutti
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.
I have not noticed assertions for JFR-based runtime metrics (io.opentelemetry.runtime-telemetry-java17
instrumentation scope). Could you please add them?
Also, could you please add more assertions for JMX-based runtime metrics (jvm.cpu.time, ...)? It woud be great to check one metric by MXBean used.
This way, we would be more confident that things work well in native mode!
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.
I have not noticed assertions for JFR-based runtime metrics (
io.opentelemetry.runtime-telemetry-java17
instrumentation scope). Could you please add them?
it's here:
Lines 26 to 32 in 7d45b5b
protected void assertAdditionalMetrics() { | |
// JFR based metrics | |
testing.waitAndAssertMetrics( | |
"io.opentelemetry.runtime-telemetry-java17", | |
"jvm.cpu.limit", | |
AbstractIterableAssert::isNotEmpty); | |
} |
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.
Also, could you please add more assertions for JMX-based runtime metrics (jvm.cpu.time, ...
here's one:
Lines 220 to 224 in 7d45b5b
// JMX based metrics | |
testing.waitAndAssertMetrics( | |
"io.opentelemetry.runtime-telemetry-java8", | |
"jvm.memory.used", | |
AbstractIterableAssert::isNotEmpty); |
or do you want to have one for each JMX bean?
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.
I would suggest to test one metric by MXBean used to verify that the MXBean can be acessible in the native mode:
Line 33 in e9227c3
OperatingSystemMXBean osBean = ManagementFactory.getOperatingSystemMXBean(); Line 36 in e9227c3
ManagementFactory.getPlatformMXBeans(BufferPoolMXBean.class); Line 33 in e9227c3
return registerObservers(openTelemetry, ManagementFactory.getMemoryPoolMXBeans());
I would aslo suggest to check a runtime metric for each JFR event used: https://github.com/search?q=repo%3Aopen-telemetry%2Fopentelemetry-java-instrumentation%20EVENT_NAME&type=code
55b2482
to
ab2439a
Compare
@@ -101,7 +101,7 @@ private JfrRuntimeMetrics(OpenTelemetry openTelemetry, Predicate<JfrFeature> fea | |||
recordingStream.onEvent(handler.getEventName(), handler); | |||
}); | |||
recordingStream.onMetadata(event -> startUpLatch.countDown()); | |||
Thread daemonRunner = new Thread(() -> recordingStream.start()); | |||
Thread daemonRunner = new Thread(recordingStream::start, "JFR-Metrics-Runner"); |
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.
Perhaps mentioned OpenTelemetry in the thread name?
@@ -31,6 +24,10 @@ public final class RuntimeMetricsBuilder { | |||
|
|||
private boolean disableJmx = false; | |||
private boolean enableExperimentalJmxTelemetry = false; | |||
private Consumer<Runnable> shutdownHook = | |||
runnable -> { | |||
Runtime.getRuntime().addShutdownHook(new Thread(runnable, "RuntimeMetricsShutdownHook")); |
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.
Perhaps mentioned OpenTelemetry in the thread name?
@@ -66,6 +66,9 @@ graalvmNative { | |||
// Workaround for https://github.com/junit-team/junit5/issues/3405 | |||
buildArgs.add("--initialize-at-build-time=org.junit.platform.launcher.core.LauncherConfig") | |||
buildArgs.add("--initialize-at-build-time=org.junit.jupiter.engine.config.InstantiatingConfigurationParameterConverter") | |||
|
|||
// enable JFR - see https://www.graalvm.org/22.0/reference-manual/native-image/JFR/ |
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.
This documentation if for an old GraalVM version.
The last one:
// enable JFR - see https://www.graalvm.org/22.0/reference-manual/native-image/JFR/ | |
// enable JFR - see https://www.graalvm.org/latest/reference-manual/native-image/debugging-and-diagnostics/JFR/ |
@roberttoyonaga If you have time, it would be great if you could have a look at this PR. |
/** | ||
* Create and start {@link RuntimeMetrics}. | ||
* | ||
* <p>Listens for select JFR events, extracts data, and records to various metrics. Recording will |
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.
I think this javadoc should be adjusted. JFR recording and metrics are only with JDK17+. This is because the JFR streaming feature only was introduced in JDK 14.
|
||
public void startFromInstrumentationConfig(InstrumentationConfig config) { | ||
/* | ||
By default, don't use any JFR metrics. May change this once semantic conventions are updated. |
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.
I think this comment should not mention JFR, since JFR is only used with JFR streaming in Java 17.
return new RuntimeMetricsBuilder(openTelemetry); | ||
} | ||
|
||
/** Stop recording JFR events. */ |
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.
Same with this javadoc
private static boolean useThreads() { | ||
// GraalVM native image does not support ThreadMXBean yet | ||
// see https://github.com/oracle/graal/issues/6101 | ||
return !isJava9OrNewer() || System.getProperty("org.graalvm.nativeimage.imagecode") != null; |
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.
I think that GraalVM Native Image would work fine with Threads::java8Callback
. Native Image implements some ThreadMXBean
functionality, notably all the functionality needed by Threads::java8Callback
(see here and here). Although, Threads::java9AndNewerCallback
still won't work with Native Image since we don't support ThreadMXBean#getAllThreadIds()
or ThreadMXBean.getThreadInfo()
yet.
Fixes #12812