You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/09/23 09:22:43 UTC
[GitHub] [bookkeeper] StevenLuMT opened a new pull request, #3501: unified latency metric unit
StevenLuMT opened a new pull request, #3501:
URL: https://github.com/apache/bookkeeper/pull/3501
Descriptions of the changes in this PR:
### Motivation
The latency of the OpStatsLogger.registerSuccessfulEvent calculation is to convert the time to milliseconds
<img width="855" alt="image" src="https://user-images.githubusercontent.com/42990025/191929953-bdac2d25-3fbe-4697-b3e9-f4942ed7c074.png">
but Counter.add nothing to do
so when using Counter for latency statistics, the time unit and OpStatsLogger are not unified,
then we unified latency metric unit
### Changes
1. change name : Counter.add --> Counter.addCount
2. add new method Counter.addLatency to count the time and convert the time to milliseconds
then how to use counter correctly:
1. when using Counter for latency metric, call Counter.addLatency
2. when using Counter for count metric, call Counter.addCount
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Shoothzj commented on pull request #3501: BP-58: Change the API for org.apache.bookkeeper.stats.Counter
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#issuecomment-1259069983
@eolivelli Sorry, My fault, I didn't realize it. Should we revert the PR now or wait for the vote result?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Shoothzj commented on a diff in pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on code in PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#discussion_r980646607
##########
bookkeeper-common/src/test/java/org/apache/bookkeeper/test/TestStatsProvider.java:
##########
@@ -60,10 +60,16 @@ public void dec() {
}
@Override
- public void add(long delta) {
+ public void addCount(long delta) {
updateMax(val.addAndGet(delta));
}
+ @Override
+ public void addLatency(long eventLatency, TimeUnit unit) {
+ long valueMillis = unit.toMicros(eventLatency) / 1000;
Review Comment:
I think we can both use unit.toMills
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on code in PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#discussion_r980647289
##########
bookkeeper-common/src/test/java/org/apache/bookkeeper/test/TestStatsProvider.java:
##########
@@ -60,10 +60,16 @@ public void dec() {
}
@Override
- public void add(long delta) {
+ public void addCount(long delta) {
updateMax(val.addAndGet(delta));
}
+ @Override
+ public void addLatency(long eventLatency, TimeUnit unit) {
+ long valueMillis = unit.toMicros(eventLatency) / 1000;
Review Comment:
> I think we can both use unit.toMills
goog idea, I will update it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Shoothzj commented on pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#issuecomment-1256042009
will this break prometheus metric result?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on code in PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#discussion_r980645336
##########
bookkeeper-common/src/test/java/org/apache/bookkeeper/test/TestStatsProvider.java:
##########
@@ -60,10 +60,16 @@ public void dec() {
}
@Override
- public void add(long delta) {
+ public void addCount(long delta) {
updateMax(val.addAndGet(delta));
}
+ @Override
+ public void addLatency(long eventLatency, TimeUnit unit) {
+ long valueMillis = unit.toMicros(eventLatency) / 1000;
Review Comment:
> Just have a question. Why not use unit.toMills?
it's the same, just use the logic with OpStatsLogger.registerSuccessfulEvent @zymap
<img width="855" alt="image" src="https://user-images.githubusercontent.com/42990025/191929953-bdac2d25-3fbe-4697-b3e9-f4942ed7c074.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#issuecomment-1257088457
> will this break prometheus metric result?
this change don't break prometheus metric result:
This just optimize the statistics,but in the end this logic still use Counter.add to update the data
@Shoothzj
<img width="1061" alt="image" src="https://user-images.githubusercontent.com/42990025/192122836-f698338d-9e32-4f19-a305-7df05099d8fd.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on code in PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#discussion_r980647289
##########
bookkeeper-common/src/test/java/org/apache/bookkeeper/test/TestStatsProvider.java:
##########
@@ -60,10 +60,16 @@ public void dec() {
}
@Override
- public void add(long delta) {
+ public void addCount(long delta) {
updateMax(val.addAndGet(delta));
}
+ @Override
+ public void addLatency(long eventLatency, TimeUnit unit) {
+ long valueMillis = unit.toMicros(eventLatency) / 1000;
Review Comment:
> I think we can both use unit.toMills
goog idea, I will update it @Shoothzj @zymap
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3501: BP-58: Change the API for org.apache.bookkeeper.stats.Counter
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#issuecomment-1259081353
> @eolivelli Sorry, My fault, I didn't realize it. Should we revert the PR now or wait for the vote result?
I have started the VOTE, see for details https://lists.apache.org/thread/4kxbdwjmls56lfmfhrf853dc525dmsy9
@Shoothzj @eolivelli @dlg99 @zymap
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on code in PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#discussion_r980651544
##########
bookkeeper-common/src/test/java/org/apache/bookkeeper/test/TestStatsProvider.java:
##########
@@ -60,10 +60,16 @@ public void dec() {
}
@Override
- public void add(long delta) {
+ public void addCount(long delta) {
updateMax(val.addAndGet(delta));
}
+ @Override
+ public void addLatency(long eventLatency, TimeUnit unit) {
+ long valueMillis = unit.toMicros(eventLatency) / 1000;
Review Comment:
@Shoothzj have a look the new update for unit.toMillis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#issuecomment-1259067049
@Shoothzj this is a PR related to a BP and we did not run the VOTE.
I will send a message on dev@
We should follow the community process
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#issuecomment-1258859655
@Shoothzj have a look the new update for unit.toMillis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] Shoothzj merged pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
Shoothzj merged PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3501: BP-58: Change the API for org.apache.bookkeeper.stats.Counter
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#issuecomment-1259654013
> @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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] zymap commented on a diff in pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#discussion_r980636993
##########
bookkeeper-common/src/test/java/org/apache/bookkeeper/test/TestStatsProvider.java:
##########
@@ -60,10 +60,16 @@ public void dec() {
}
@Override
- public void add(long delta) {
+ public void addCount(long delta) {
updateMax(val.addAndGet(delta));
}
+ @Override
+ public void addLatency(long eventLatency, TimeUnit unit) {
+ long valueMillis = unit.toMicros(eventLatency) / 1000;
Review Comment:
Just have a question. Why not use unit.toMills?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] StevenLuMT commented on pull request #3501: unified latency metric unit
Posted by GitBox <gi...@apache.org>.
StevenLuMT commented on PR #3501:
URL: https://github.com/apache/bookkeeper/pull/3501#issuecomment-1258861232
@Shoothzj have a look the new update for unit.toMillis
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org