Skip to content

Extract JFR recording stream configuration into JfrMeterBinder#7531

Open
schiemon wants to merge 1 commit into
micrometer-metrics:mainfrom
schiemon:copy-shared-jfr-meter-binder
Open

Extract JFR recording stream configuration into JfrMeterBinder#7531
schiemon wants to merge 1 commit into
micrometer-metrics:mainfrom
schiemon:copy-shared-jfr-meter-binder

Conversation

@schiemon

Copy link
Copy Markdown
Contributor

This PR introduces JfrMeterBinder to unify RecordingStream configuration and JFR event handler registration for JFR-backed MeterBinders.

JfrMeterBinder was put into an newly added micrometer-java17 module as RecodingStream was introduced in Java 14 and Java 17 is the next-in-line LTS version.

This is also pre-work for #7384 in which we are going to introduce JvmSafepointMetrics, after VirtualThreadMetrics the second JfrMeterBinder in Micrometer.

@schiemon schiemon force-pushed the copy-shared-jfr-meter-binder branch 2 times, most recently from e1a4093 to 402fbb5 Compare May 26, 2026 10:32
* @param eventHandlerRegistrar the registrar to use for registering handlers for JFR
* events
*/
protected abstract void bindTo(MeterRegistry registry, EventHandlerRegistrar eventHandlerRegistrar);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am intentionally not simply handing over the RecordingStream or EventStream as I don't want to allow the implementation to start or close the stream.

Signed-off-by: szymon.habrainski <habrainskiszymon@gmail.com>
@schiemon schiemon force-pushed the copy-shared-jfr-meter-binder branch from 402fbb5 to 28203e5 Compare May 26, 2026 10:58
@schiemon schiemon marked this pull request as ready for review May 26, 2026 11:09
@jonatan-ivanov

Copy link
Copy Markdown
Member

@shakuzen and I need to discuss if we want to generalize the JFR binders in the next minor release. I think we should but I'm in favor of a simpler approach, like the code you copied in this PR: main...jonatan-ivanov:micrometer:jfr-binder

@schiemon

schiemon commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

@shakuzen and I need to discuss if we want to generalize the JFR binders in the next minor release.

Alright, please notify me with your decision when you are ready 👍

but I'm in favor of a simpler approach, like the code you copied in this PR: main...jonatan-ivanov:micrometer:jfr-binder

Personally, I also like that it is simple but dislike that it leaks the RecordingStream to the implementation because then, in principle, the implementation could start or stop and reconfigure the stream. IMHO the JfrBinder should completely own the stream. But again, please let me know how you guys decide so I can move on with this one and #7384

@schiemon

schiemon commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@jonatan-ivanov friendly bump :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants