Virtual threads metrics

[ArtyomGabeev]

Fixes #3956

[shakuzen]

This looks generally good. Have you tried this out in an application yet? What is remaining for this to be ready to merge, you think? I couldn't see what the build failure was perhaps because it is too old at this point. Could you rebase on the latest main and update this branch so a new build runs?

It'd be good to have a test for this also. You can make the test disabled on Java versions before 21.

Copying from the issue to keep the discussion here where changes would be made:

I think this instrumentation should be specific to java-21. Should I go and create a java-21 source set or separate gradle module?

Is there any API used that isn't available in Java 8? If it is only a runtime issue of the specific JFR events won't be available except on Java 21 and later, I don't think we need to do anything special with the source or making a separate module. It will be up to users to not bind the MeterBinder if they are not on Java 21 or later.

I'm not sure we need to process jdk.VirtualThreadStart and jdk.VirtualThreadEnd events.

Agreed. I think we can ignore those because users can instrument run time of virtual threads via instrumenting the executor as you said or they can instrument the execution of a single virtual thread created ad hoc.

For [VirtualThreadPinnedEvent], things to consider:

1. Add a tag with stackframe where pinning happened? e.g.: com.test.ThreadPinningExample.sleep(Duration)
2. Additionally, I would like to be able to perform log once with extended strack trace, but I'm not sure this is a responsibility of micrometer library.

Logging a stack trace can be enabled with the system property jdk.tracePinnedThreads - see this documentation. So I don't think we should/need to do anything in Micrometer for that.

As for tagging the method signature where the pinning happened, I'm not sure. I think I would err on the side of not including it initially. The cardinality of it is hard to know and application-specific. Without that tag, users will know how much pinning is happening and for how long, at least. Maybe that provides enough value. We can consider adding it after getting some feedback on the metrics without it. Thoughts?

[ArtyomGabeev]

Hi @shakuzen ,

I've been using this meter binder for a couple of months already.

I think the JFR streaming API is available only from JDK 14; that's why I've asked about a separate module for jdk21. Additionally, covering it with unit tests is easier on jdk21 since we have access to virtual threads builders.

I agree that neither logging pinned stack trace nor tagging with class caused pinning is incorrect for meter binder implementation.

[shakuzen]

Left some review on the implementation. I'll discuss with @jonatan-ivanov about making a new java 21+ module for this and get back to you.

[shakuzen]

I'm not familiar with the right RecordingStream usage, but I was looking at this blog, and it made me wonder: do we not need to start the stream? And should we not set a max age so we don't accumulate events indefinitely?

[ArtyomGabeev]

hey @shakuzen,

Do you think we need to expose maxAge duration as a constructor parameter?

[shakuzen]

Good question. I had the same thought, but I'm not sure due to my lack of familiarity with JFR and in particular RecordingStream. I don't understand what this max age means in terms of the event listeners we register. I assume the event listeners are run nearly instantly after an event is recorded. So I'm not sure the max age could affect them. Or will max age effectively filter out events with a duration longer than the max age duration?

[shakuzen]

I talked to @jonatan-ivanov about where to put this code. I think a new micrometer-java21 module is probably the least bad solution unless someone has other suggestions. @ArtyomGabeev would you update the PR to move this to such a module? You can see the micrometer-java11 module for something similar that already exists.

[ArtyomGabeev]

I talked to @jonatan-ivanov about where to put this code. I think a new micrometer-java21 module is probably the least bad solution unless someone has other suggestions. @ArtyomGabeev would you update the PR to move this to such a module? You can see the micrometer-java11 module for something similar that already exists.

I've extracted micrometer-java21 module and have a couple of questions:

1. I've removed jdk11 and jdk17 CI executors since we are building and publishing under jdk21.
   However, strategically we may still want to execute unit tests under jdk11/17 and conditionally add new module in gradle settings. Note: docker tests will be run only on jdk21.
2. What is the minimum supported java version? If it is 8, we are not running any builds right now on jdk8.
3. If the minimum supported java version is 11, we can do some clean up in separate PR and remove java11 specific sourcesets from micrometer-core module, clean up the docs and gradle scripts. This is unrelated to this PR, but I can take a look later.

[shakuzen]

Rather than remove any CI executors, see what is done in the micrometer-jetty12 module:

micrometer/micrometer-jetty12/build.gradle

Lines 3 to 6 in 9917c67

	// skip this module when building with jdk <17
	if (!javaLanguageVersion.canCompileOrRun(17)) {
	project.tasks.configureEach { task -> task.enabled = false }
	}

• What is the minimum supported java version? If it is 8, we are not running any builds right now on jdk8.

It is 8. It would be nice if we still have a JDK 8 build but it became a pain to maintain that due to test dependencies. The intention is that using the -release option set to 8 should catch compatibility issues.

[ArtyomGabeev]

Rather than remove any CI executors, see what is done in the micrometer-jetty12 module:

micrometer/micrometer-jetty12/build.gradle

Lines 3 to 6 in 9917c67

	// skip this module when building with jdk <17
	if (!javaLanguageVersion.canCompileOrRun(17)) {
	project.tasks.configureEach { task -> task.enabled = false }
	}

• What is the minimum supported java version? If it is 8, we are not running any builds right now on jdk8.

It is 8. It would be nice if we still have a JDK 8 build but it became a pain to maintain that due to test dependencies. The intention is that using the -release option set to 8 should catch compatibility issues.

Done.

[shakuzen]

I think overall this looks good. I'll try to take a look at some prior art on instrumenting metrics with JFR to make sure we aren't missing any best practices. I left a review comment more for the maintainers to consider.
@jonatan-ivanov any concerns with this PR?

[jonatan-ivanov]

@ArtyomGabeev @shakuzen I fixed the comments I made on a branch except the package and class rename, see first commit here: ArtyomGabeev/micrometer@virtual-threads-metrics...jonatan-ivanov:micrometer:virtual-threads-metrics

I also found a way to test starting and unparking failures and receive JFR events for them. Unfortunately this is a hack with reflection. Even more unfortunate that the JDK tests are doing the same, see the comments of the test.

[ArtyomGabeev]

@ArtyomGabeev @shakuzen I fixed the comments I mage on a branch except the package and class rename, see first commit here: ArtyomGabeev/micrometer@virtual-threads-metrics...jonatan-ivanov:micrometer:virtual-threads-metrics

I also found a way to test starting and unparking failures and receive JFR events for them. Unfortunately this is a hack with reflection. Even more unfortunate that the JDK tests are doing the same, see the comments of the test.

Hi, @jonatan-ivanov
I've applied the proposed changes except for unparking failure tests and renamed metrics and the base package.
Thanks,
Artyom.

[jonatan-ivanov]

Thank you!
Fyi: I added the test case for the failed events, let's see if maintenance will be a pain and if we can improve it in the future.

[jonatan-ivanov]

@ArtyomGabeev @shakuzen I separated the reflective tests, see the last commit and let me know what you think: 11f1016