BP-58: Change the API for org.apache.bookkeeper.stats.Counter #3501
Conversation
|
will this break prometheus metric result? |
this change don't break prometheus metric result: |
| updateMax(val.addAndGet(delta)); | ||
| } | ||
|
|
||
| @Override | ||
| public void addLatency(long eventLatency, TimeUnit unit) { | ||
| long valueMillis = unit.toMicros(eventLatency) / 1000; |
There was a problem hiding this comment.
Just have a question. Why not use unit.toMills?
There was a problem hiding this comment.
Just have a question. Why not use unit.toMills?
it's the same, just use the logic with OpStatsLogger.registerSuccessfulEvent @zymap
There was a problem hiding this comment.
I think we can both use unit.toMills
There was a problem hiding this comment.
@shoothzj have a look the new update for unit.toMillis
|
@shoothzj have a look the new update for unit.toMillis |
|
@shoothzj this is a PR related to a BP and we did not run the VOTE. We should follow the community process |
eolivelli
changed the title
|
@eolivelli Sorry, My fault, I didn't realize it. Should we revert the PR now or wait for the vote result? |
@shoothzj don't revert ,see the detail for mail discussion: https://lists.apache.org/thread/zyj3z3tt4bsx928mc0ogfkv63w9t47bc |
Descriptions of the changes in this PR:
Motivation
The latency of the OpStatsLogger.registerSuccessfulEvent calculation is to convert the time to milliseconds
but Counter.add nothing to do
so when using Counter for latency statistics, the time unit and OpStatsLogger are not unified, which is easy to be misleading.
then we unified latency metric unit
Changes
then how to use counter correctly: