-
Notifications
You must be signed in to change notification settings - Fork 521
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
Publicise the mbean traits #3813
base: series/3.x
Are you sure you want to change the base?
Conversation
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 way they could be gotten/invoked via the java ManagementFactory / MBeanServer.
😅 how does one do this exactly? Might be a good snippet to add to (scala)doc to explain how to use these now-public interfaces.
private[metrics] trait CpuStarvationMBean { | ||
trait CpuStarvationMBean { |
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.
Interesting, this one is in c.e.metrics
package ...
private[unsafe] trait ComputePoolSamplerMBean { | ||
trait ComputePoolSamplerMBean { |
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.
And the rest are in c.e.unsafe.metrics
. I can see why (these metrics are on unsafe things) but I wonder if now that we are making it public we should make it more uniform. Not sure.
Also: should we seal these traits? That way we can add new things to them without breaking compatibility. e.g. ComputePoolSamplerMBean
might get polling-system metrics in the future?
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.
In my PR #3317, I moved all metrics to the cats.effect.unsafe.metrics
package.
But are metrics really unsafe? 🤔
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.
Btw, once we make *MBean
traits public, my PR will be binary breaking, right?
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 agree. I think we should put it all under one package.
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.
But are metrics really unsafe?
Technically yes: they are side-effectual interfaces.
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 can move the
CpuStarvationMBean
intounsafe.metrics
for sure - same with sealing the traits, that makes sense to me!
@iRevive I don't want to make your changes difficult!! What is the status of your PR? Would it be better if I hold off on the publicising until after your changes are merged in?
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.
@samspills I've updated my PR (merged the upstream).
The purpose of the PR is to make the metrics publicly available, so you can integrate them with third-party libraries (or systems).
For example, the Prometheus collector can publish compute metrics without relying on mbeans. Here is an oversimplified example #3317 (comment).
Or otel4s can get a native integration.
Oh, btw this PR should be targeted to |
1c852f1
to
185a2d5
Compare
Can we merge this with HEAD so we get the CI calming changes? |
oh sorry about that! thanks @armanbilge <3 |
b1affb8
to
5b7d873
Compare
I think I've gotten everything publicised correctly but I still can't trigger it via an mbean proxy because of the hash that gets added to the bean name here cats-effect/core/jvm/src/main/scala/cats/effect/unsafe/IORuntimeCompanionPlatform.scala Line 269 in 8e38eda
(I'm using the fiber snapshot as an example, but I believe all mbeans have a hash added to the name EDIT: for clarity, all cats-effect mbeans. Other mbeans I've looked at don't seem to follow this convention) With that hash in place I still have to get the mbeans using a query like I did originally. Maybe I'm just not familiar with mbeans (fact), but it's not clear to me why the hash is necessary? @armanbilge do you know? If it were removed then accessing the bean is a bit nicer: def readFibersFromRuntimeNoHash: IO[String] = IO {
val fiberObjectName =
new ObjectName("cats.effect.unsafe.metrics:type=LiveFiberSnapshotTrigger")
val server = ManagementFactory.getPlatformMBeanServer
val fiberBean = JMX.newMBeanProxy(
server,
fiberObjectName,
classOf[LiveFiberSnapshotTriggerMBean]
)
fiberBean.liveFiberSnapshot().mkString
} Or maybe I'm missing a better way to work with the mbeans? |
If I had to guess: in case there are two or more |
This way they could be gotten/invoked via the java ManagementFactory / MBeanServer.
I'd like to follow this up by investigating #3038 and I'm pretty sure this won't interfere there but I'm mentioning it here in case I'm wrong 😅
cc @mpilquist