Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable Virtual Thread Binder if micrometer-java21 is on the Classpath #44686

Merged
merged 1 commit into from
Nov 28, 2024
Merged

Enable Virtual Thread Binder if micrometer-java21 is on the Classpath #44686

merged 1 commit into from
Nov 28, 2024

Conversation

cescoffier
Copy link
Member

This commit introduces automatic registration of the virtual thread meter binder when the io.micrometer:micrometer-java21 dependency is present. The binder collects metrics related to virtual threads pinning and misbehavior (unable to unpark or start)

The binder is activated under the following conditions:

  • The micrometer-java21 dependency is available on the classpath.
  • The application is running on Java 21 or higher.
  • The quarkus.micrometer.binder.virtual-threads.enabled property is set to true (default).

@cescoffier cescoffier requested a review from brunobat November 25, 2024 13:50
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 25, 2024

/cc @brunobat (micrometer), @ebullient (micrometer)

@cescoffier
Copy link
Member Author

@brunobat Hey! I mentioned that work with you last week. Here is an initial draft.
The idea is to register the binder introduced in micrometer-metrics/micrometer#5067.

@cescoffier cescoffier added the area/virtual-threads Issue related to Java's Virtual Threads label Nov 25, 2024
brunobat
brunobat reviewed Nov 25, 2024
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me @cescoffier. Will you be adding something else to the PR?

@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 25, 2024 •
edited
Loading

🙈 The PR is closed and the preview is expired.

@cescoffier
Copy link
Member Author

@brunobat Nope,all good on my side. Let me open it for review if you don't see any big problem.

@cescoffier cescoffier marked this pull request as ready for review November 25, 2024 17:15
@gsmet gsmet requested a review from brunobat November 27, 2024 17:28
brunobat
brunobat requested changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need an IT test

@cescoffier
Copy link
Member Author

@brunobat Good point about the IT:

Caused by: java.lang.UnsatisfiedLinkError: jdk.jfr.internal.JVM.getPid()Ljava/lang/String; [symbol: Java_jdk_jfr_internal_JVM_getPid or Java_jdk_jfr_internal_JVM_getPid__]
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.jni.access.JNINativeLinkage.getOrFindEntryPoint(JNINativeLinkage.java:152)
        at org.graalvm.nativeimage.builder/com.oracle.svm.core.jni.JNIGeneratedMethodSupport.nativeCallAddress(JNIGeneratedMethodSupport.java:54)

@cescoffier
Copy link
Member Author

@brunobat It requires quarkus.native.monitoring=jfr. Updating documentation.

@cescoffier cescoffier force-pushed the virtual-thread-metrics branch 2 times, most recently from cb54f92 to f317787 Compare November 27, 2024 18:34
@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Nov 27, 2024
@cescoffier
Copy link
Member Author

@brunobat IT added and doc updated.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
brunobat
brunobat requested changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check my comments bellow.

docs/src/main/asciidoc/virtual-threads.adoc Outdated Show resolved Hide resolved
...tion-tests/virtual-threads/metrics-virtual-threads/src/main/resources/application.properties Outdated Show resolved Hide resolved
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@cescoffier
Enable Virtual Thread Binder if micrometer-java21 is on the Classpath
feae06c 
This commit introduces automatic registration of the virtual thread meter binder when the `io.micrometer:micrometer-java21` dependency is present. The binder collects metrics related to virtual threads pinning and misbehavior (unable to unpark or start)

The binder is activated under the following conditions:
- The `micrometer-java21` dependency is available on the classpath.
- The application is running on Java 21 or higher.
- The `quarkus.micrometer.binder.virtual-threads.enabled` property is set to true (default).
@cescoffier cescoffier force-pushed the virtual-thread-metrics branch from f317787 to feae06c Compare November 28, 2024 06:48
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 28, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit feae06c.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

brunobat
brunobat reviewed Nov 28, 2024
View reviewed changes
Copy link
Contributor

@brunobat brunobat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks @cescoffier !

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 28, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit feae06c.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@brunobat brunobat merged commit d1c1e91 into quarkusio:main Nov 28, 2024
55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Nov 28, 2024
@cescoffier cescoffier deleted the virtual-thread-metrics branch December 2, 2024 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunobat brunobat brunobat approved these changes

Assignees
No one assigned
Labels
area/core area/documentation area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/metrics area/virtual-threads Issue related to Java's Virtual Threads
Projects
None yet
Milestone
3.18.0.CR1
Development

Successfully merging this pull request may close these issues.
2 participants
@cescoffier @brunobat