-
Notifications
You must be signed in to change notification settings - Fork 2
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
runtime delegation to JFR streaming APIs via reflection #223
Conversation
Generate changelog in
|
|
||
@Test | ||
public void testOpenRepository() { | ||
assertThat(EventStreams.openRepository()).isPresent(); |
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.
do we need to close resources here?
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 suppose not, since it's only reading a view of the default repository
AtomicBoolean seen = new AtomicBoolean(false); | ||
support.get().onEvent("jdk.ActiveRecording", (_ev) -> seen.compareAndSet(false, true)); | ||
|
||
support.get().startAsync(); |
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 we need to terminate this in some way
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.
done
}); | ||
} | ||
|
||
public interface RecordingStreamSupport extends AutoCloseable { |
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 we should declare @Override void close()
to narrow close
from AutoCloseable
not to throw a checked exception, matching RecordingStream.close
.
This makes it easier to do something like the follwoing, since you can't throw checked exceptions within a functional method:
RecordingStreams.newRecordingStream()
.ifPresent(recordingStream -> {
try (recordingStream) {
///etc
}
});
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.
Looks like the same applies to EventStream as well
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.
done
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.
lgtm, thanks!
Released 0.4.0 |
Adds support for accessing JFR streaming APIs at runtime via reflection, while still supporting language levels below Java 14, in a manner similar to the
VirtualThreads
class from https://github.com/palantir/nylon-threads.This allows consumers to access JFR streaming APIs when running on Java 14 or above, while still targeting language levels below Java 14. The
EventStreams
andRecordingStreams
classes provide factory methods for creating an object similar in shape tojdk.jfr.consumer.EventStream
orjdk.jfr.consumer.RecordingStream
respectively, which delegates to an underlying instance at runtime. If JFR streaming is not supported by the runtime, anOptional.empty()
is returned instead.Note that
EventStreams
does not provide support for theonMetadata
method ofjdk.jfr.consumer.EventStream
, as it depends upon a class added in Java 16. Similarly,RecordingStreams
does not provide support forjdk.jfr.consumer.RecordingStream#stop()
as it was added in Java 20 and I'm making the assumption that most consumers are still targeting Java 17 or below.