ZOOKEEPER-4983: Add client-triggered operation count metrics #2328
Conversation
li4wang
force-pushed
the
ZOOKEEPER-4983
branch
4 times, most recently
from
2b44c44 to
436ef46
Compare
li4wang
force-pushed
the
ZOOKEEPER-4983
branch
4 times, most recently
from
16b0d7b to
b37eefd
Compare
|
@eolivelli @kezhuw @anmolnar Would you mind reviewing the PR? Thanks |
li4wang
force-pushed
the
ZOOKEEPER-4983
branch
from
b37eefd to
71de08e
Compare
li4wang
force-pushed
the
ZOOKEEPER-4983
branch
from
71de08e to
3f0e2d4
Compare
| /** | ||
| * Operation count metrics | ||
| */ | ||
| TOTAL_OP_COUNT = metricsContext.getCounter("total_op_count"); |
There was a problem hiding this comment.
what about adding a common prefix to all these metrics ?
like "frp_" (final request processor) or "final_request_processor_"
This way the metrics are naturally grouped (and when you explore them with Grafana or build a dashboard your life is much easier)
There was a problem hiding this comment.
Good point!
How about using op_cout as the prefix, so they are grouped and also the name is more concise and readable?
PDavid
left a comment
PDavid
left a comment
There was a problem hiding this comment.
Many thanks for this improvement, looks really nice already. 👍
I just added some minor questions / comments.
| /** | ||
| * Operation count metrics | ||
| */ |
There was a problem hiding this comment.
Just a small / nit question:
Why do we need a JavaDoc comment here for this? I guess this would be some kind of a divider here and we have some examples above, but in this case shouldn't this be a normal (not JavaDoc comment)? 🤔
(I can see that there are already some examples of this above but I think those are also most probably wrong.)
There was a problem hiding this comment.
Good catch. Yes, we don't need JavaDoc comment for this. Updated
| /** | ||
| * Operation count metrics | ||
| */ |
There was a problem hiding this comment.
Here as well. Do we need to have this as JavaDoc comment or rather some kind of divider? 🤔
As is right now the JavaDoc (documentation) of TOTAL_OP_COUNT field will be "Operation count metrics". Is this desired?
If we need some kind if divider would it maybe make sense to use a normal (not JavaDoc comment)?
I see that there are mixed examples in this class already.
What do you all think?
There was a problem hiding this comment.
Very good point. Use the normal comment would be appropriate here. Updated
| import org.apache.zookeeper.test.ClientBase; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| public class ServerMetricsOpCountTest extends ClientBase { |
There was a problem hiding this comment.
Many thanks for adding the test for this. 👍
li4wang
force-pushed
the
ZOOKEEPER-4983
branch
from
adf28a5 to
6d4bec1
Compare
li4wang
force-pushed
the
ZOOKEEPER-4983
branch
from
6d4bec1 to
d045a94
Compare
|
@eolivelli @PDavid Thanks for reviewing the PR and providing valuable comments. I've addressed all of them. Would you mind taking a look at it? |
li4wang
requested review from
PDavid
and removed request for
anmolnar and
kezhuw
3 participants
This PR contains the following changes for supporting client-triggered op count metrics for more visibility of client triggered load and pattern.