Conversation
armanbilge
left a comment
There was a problem hiding this comment.
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.
| * An MBean interfaces for monitoring when CPU starvation occurs. | ||
| */ | ||
| private[metrics] trait CpuStarvationMBean { | ||
| trait CpuStarvationMBean { |
There was a problem hiding this comment.
Interesting, this one is in c.e.metrics package ...
| * An MBean interface for monitoring a [[WorkStealingThreadPool]] backed compute thread pool. | ||
| */ | ||
| private[unsafe] trait ComputePoolSamplerMBean { | ||
| trait ComputePoolSamplerMBean { |
There was a problem hiding this comment.
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.
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.
Btw, once we make *MBean traits public, my PR will be binary breaking, right?
There was a problem hiding this comment.
I agree. I think we should put it all under one package.
There was a problem hiding this comment.
But are metrics really unsafe?
Technically yes: they are side-effectual interfaces.
There was a problem hiding this comment.
- I can move the
CpuStarvationMBeanintounsafe.metricsfor 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.
@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 (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 |
|
|
||
| /** | ||
| * Returns the index into the circular buffer backing the monitored [[LocalQueue]] which | ||
| * Returns the index into the circular buffer backing the monitored `LoLcalQueue` which |
There was a problem hiding this comment.
| * Returns the index into the circular buffer backing the monitored `LoLcalQueue` which | |
| * Returns the index into the circular buffer backing the monitored `LocalQueue` which |
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