Tidy up jfr-streaming #127
Conversation
|
/cc @kittylyst |
check out #121 (comment) (maybe add a comment on those methods so we don't keep trying to remove them 😄) |
Interesting! @kittylyst what hooks into the |
No, we should not differentiate that way. Instead read/write need to be an attribute. The conflict is likely due to the description/unit being different. To the extent we register one instrument and "bind" those attributes, that'd be ideal. |
| private static final String METRIC_NAME_DURATION = "runtime.jvm.network.write.duration"; | ||
| private static final String METRIC_NAME_BYTES = "runtime.jvm.network.write.io"; | ||
| private static final String DESCRIPTION_BYTES = "Bytes Written"; | ||
| private static final String DESCRIPTION_DURATION = "Write Duration"; |
There was a problem hiding this comment.
This is likely the issue between duration read. This needs to be a more generic description about duration of a network operation.
There was a problem hiding this comment.
Cool yeah that did the trick 👍
Fixed in most recent commit.
| private BoundDoubleHistogram machineHistogram; | ||
| private final BoundDoubleHistogram userHistogram; | ||
| private final BoundDoubleHistogram systemHistogram; | ||
| private final BoundDoubleHistogram machineHistogram; | ||
|
|
||
| public OverallCPULoadHandler(Meter otelMeter) { |
There was a problem hiding this comment.
If we revert the removal of init I think we can merge this and follow up on that. Quarkus preinitializing classes was the reason to have an init method - ideally we can find some way to improve, possibly on the Quarkus side, to avoid forcing the pattern on us but I think we can try that later
There was a problem hiding this comment.
I'm fine with doing that I just want to understand what problem it solves. It would make sense to me if the init methods accepted a Meter, so that the metric instruments could be initialized after the metric sdk was setup, but that wasn't happening.
| meter | ||
| .histogramBuilder(METRIC_NAME) | ||
| .setDescription(DESCRIPTION) | ||
| .setUnit(ONE) |
There was a problem hiding this comment.
Noticed that user CPU usage unit is percentage while system and machine CPU usage unit is "1". Seems off.
There was a problem hiding this comment.
the unit for user CPU usage was just updated in #140, probably just missed it for system and machine
Was going through the the new jfr-streaming module and noticed some things I thought could use tidying up. In summary:
init()method.init()methodvarwhere appropriate.private.AbstractThreadDispathingHandler#createPerThreadSummarizerto returnConsumer<RecordedEvent>. The difference in each thread is just in how they consumer recorded events - no need for each of these responses to require their owngetEventName()implementation.runtime.jvm.network.durationandruntime.jvm.network.io. Determined that the read and write handlers both were creating instruments with the same name. I differentiated them to include read / write in the metric name, eg, ``runtime.jvm.network.write.duration`. (On a related not, @jsuereth, these instruments had the same name but different descriptions, yet still conflicted. Does this match your expectations?)