-
Notifications
You must be signed in to change notification settings - Fork 7.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ZOOKEEPER-3098: Add additional server metrics #580
Conversation
Finally this happens, thanks for contributing the change back @jtuple . My quick comments inline. In addition, there are two things:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jtuple
Although I'm more than happy to see you're trying to improve and extend Zookeeper metrics system, I have the following concerns with this patch:
- It reinvents the wheel by implementing counters like
AvgMinMaxCounter
andSimpleCounter
. Would you please elaborate on the benefits of implementing our versions rather than using some existing library? - It doesn't add any unit tests for these new types.
See my PR #440 where I tried to introduce usage of dropwizard
library. I don't want it to be committed now, I'd like to wait for @eolivelli to finish his work on the common framework, but I still believe that using Dropwizard or any other industry-standard metrics library is better for the long term.
I have a feeling that these specific libraries are doing far better at this area that we ever could. Remember the Unix/Linux philosophy: "do one thing and do it well" and ZooKeeper is not a metrics library.
@eolivelli @jtuple |
I'm happy to add tests for We actually landed a version of #440 internally at Facebook and have been running with Dropwiz-based percentile counters in production for a few months now. We build on the same In practice, the memory overhead for the Dropwiz histogram is much higher than the flat We also have In any case, happy to rework this PR with guidance and have the bandwidth to do so if we can come to consensus on a design. My biggest concern is that pretty much all of our remaining internal features at Facebook that we want to upstream are blocked on this feature, since we aggressively add metrics when adding new features (so that we can monitor/track in production, etc). The sooner we can land something, the sooner we can rebase on top of whatever the agreed approach is and unblock our other contributions. Open to any design that the community is happy with, however I think there's a few things which are important to maintain:
|
@jtuple in my opinion it is better to allow all of changes coming from Facebook to be merged in as soon as possible. So from my point of view it will be better to mark cleearly all of the changes coming from FB fork, this way reviewers and committer will take it in account and treat the patch under that light. Last year for instance in Bookkeeper we merged three 'diverged' forks from Yahoo, Twitter and Salesforce into the Apache codebase. It has been a long work but now it is super easy to collaborate. Once we have a common codebase refactoring will be easier for everyone. Just my 2cents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed context. I also really like the idea of backporting Facebook's changes into the main codebase and I believe we should support the initiative and make every effort to ease future contributions.
Let's put it this way. @eolivelli What are the main differences between this PR and your approach? Is there anything missing here which is part of your work and you think it's essential. I see the followings to consider at very high level.
- As @jtuple mentioned this code is already tested and proven in production,
- Interface system is much simpler: it only introduced
Metric
essentially. Neither we haveMetricsProvider
norMetricsContext
. Do we really need them? - ServerMetrics is a nice approach to gather all supported metrics with a common interface.
private long minLatency = Long.MAX_VALUE; | ||
private long totalLatency = 0; | ||
private long count = 0; | ||
private final AtomicLong packetsSent = new AtomicLong(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these 3 not part of ServerMetrics
?
Is that a future step of the migration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's being counted in requestLatency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I see what you mean by "being counted"?
I've thought that these three metrics should be part of ServerMetrics as:
- packetsSent and packetsReceived as SimpleCounter,
- requestLatency as AvgMinMaxCounter
and should be referred as ServerMetrics.PACKETS_SENT, etc. like the rest.
Please let me know if didn't get it right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When making this change, we kept all existing metrics "as-is" and only added new metrics to ServerMetrics
. The packet sent/receive and request_latency are both examples of metrics existing prior to this pull-request which are directly exposed in both in the /metrics
admin command (where ServerMetrics
metrics also report) as well as the mntr
four letter command (where ServerMetrics
metrics do not report).
The only changes to these existing metrics in this pull-request was converting some of them to AtomicLong
for minor performance wins.
I'm happy to move both of these metrics to ServerMetrics
if we want. I'm not sure if the names will 100% match the current values though. We'd also need to decide if we're happy losing these metrics in mntr
or if I should port the existing mntr
reporting logic to still query these from ServerMetrics
.
This is also an open question for all other pre-existing metrics in ServerStats
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ServerMetrics
is a superset of ServerStats
in terms of scope, we probably want to keep ServerStats
as is and ultimately deprecate it in favor of ServerMetrics
. I don't think there is a need to duplicate metrics in two places, which would be both a burden to maintain and a potential source of confusion.
Regarding reporting to mntr
, we decided deprecate 4lw last year due to the limitation of its design, in particular around security, in favor of admin server endpoints (/metrics
in this case), so I don't there is a need to report newly added metrics to mntr
. This also encourages users to migrate away from 4lw to admin end points.
Overall the state in current patch w.r.t this looks good to me.
@anmolnar this change is a backport from Facebook fork. in PR #582 I have started a brand new work, to design and implement a long term ZK internal API to instrument server and client java code base. This great work from @jtuple and his collegues will be (quite) easily refactored to the new system once we have merged Facebook contribs and merged the new system. On the new Metrics API the most interesting key points are:
I would like to move forward and send the next patches but we need to have agreement on the high level decisions |
@eolivelli My point is that we should get these 2 initiatiatives as close as possible and minimize the need for refactoring on both sides.
I'm not sure that we can get to that point in the short term, hence I encourage to do these 2 things side by side. |
@anmolnar If I understand correctly, @eolivelli's work is more about making it easier to export ZooKeeper's internal metric to external systems, while FB is focusing on adding a lot more useful new metrics, so they don't conflict with each other that much. And even we need to change something to adapt the new metric framework it should be trivial. |
I think it will be easier for FB guys to merge their work to community repo and then we can move to the new system. So I will lean towards merging all the FB work about metrics and then convert to the new system. In parallel we can take decisions about the new system, we can merge my changes as long as they do not have significant impact at runtime.. For me it would be a nice approch to let @lvfangmin convert eventually the instrumentation points to the new system. I can focus on implementation of providers, like Prometheus and Dropwizard |
@lvfangmin @eolivelli Thanks for the clarification. Let's do it the way you're suggesting. |
@jtuple do you have time to resolve the comments and rebase this onto latest branch? It would be great if we can get this in this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to get the ball rolling again here, because it's very close to be committed.
private long minLatency = Long.MAX_VALUE; | ||
private long totalLatency = 0; | ||
private long count = 0; | ||
private final AtomicLong packetsSent = new AtomicLong(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I see what you mean by "being counted"?
I've thought that these three metrics should be part of ServerMetrics as:
- packetsSent and packetsReceived as SimpleCounter,
- requestLatency as AvgMinMaxCounter
and should be referred as ServerMetrics.PACKETS_SENT, etc. like the rest.
Please let me know if didn't get it right.
We at Twitter manage large ZK clusters and we also have our own internal metrics system (you can't really operate things like ZK without metrics at large scale.). Our design is similar like this in terms of metrics collection and code instrumentation, but we coupled the metrics collection with publication. We are publishing metrics through Finagle (Twitter's RPC library) and the tight coupling caused several issues, such as circular dependencies between Finagle and ZooKeeper, where ZK depends on Finagle for metrics, and Finagle depends on ZK for ServerSets (naming resolution). I think the current design proposed in the community would solve this problem. Basically we will have two systems:
So IMO both work are sort of orthogonal and can be done in parallel. And I agree with the design decisions @jtuple proposed and @eolivelli commented previously. Regarding use an external metrics type system: I think it would be more flexible for us to define and own the metrics definitions so it's easy to add / change the system without depends on third parties. |
One question about gathering metrics using this system: let's say I have a use case where I want to gather the number of requests processed by FinalRequestProcessor for each request type (e.g. Ping, CreateSession, and other OpCode types). How to do this using this metrics system? I think I have to add, for each OpCode, a new enum type in ServerMetrics, is this correct? |
And yes like others commented, please rebase and resolve conflicts, and reply to previous unattended comments. My previous comments need address are:
|
e6935f8
to
f1ae575
Compare
Sorry for the delay, August was a super busy month. I've rebased onto latest master + added basic unit tests for I've also addressed the majority of inline comments. Please let me know if there's anything else I overlooked. As mentioned in a prior comment, this pull request is just the first of many. In this PR we add the We have additional metrics internally that we'll upstream in the future, including Those are all the types we currently have internally. Other types like a Gauge could easily be done in a future pull-request, although our experience has been that gauge's are less useful in a polling based approach since you only see the most recent value. Something like an For things like commit processor queue sizes and the like, we simply query the queue size every time we enqueue an item and report that instantaneous size as a value in an @hanm: For your example, you have a few options:
|
Thanks for detailed reply, @jtuple. Jenkins was not happy because one C client test failed (the 'Test Result' only shows Java tests). I kicked a
OK.
I think this is what I was looking for - a way of organizing metrics hierarchically like files/folder or namespaces. Good to know it's already supported and will be upstreamed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just had another question around the instrumentation point of FOLLOWER_SYNC_TIME
. Other than this, the patch looks good to me.
syncWithLeader(newEpochZxid); | ||
} finally { | ||
long syncTime = Time.currentElapsedTime() - startTime; | ||
ServerMetrics.FOLLOWER_SYNC_TIME.add(syncTime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will execute regardless of syncWithLeader
succeeded or not. Should we only collect syncTime
for the syncs that were successful?
private long minLatency = Long.MAX_VALUE; | ||
private long totalLatency = 0; | ||
private long count = 0; | ||
private final AtomicLong packetsSent = new AtomicLong(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ServerMetrics
is a superset of ServerStats
in terms of scope, we probably want to keep ServerStats
as is and ultimately deprecate it in favor of ServerMetrics
. I don't think there is a need to duplicate metrics in two places, which would be both a burden to maintain and a potential source of confusion.
Regarding reporting to mntr
, we decided deprecate 4lw last year due to the limitation of its design, in particular around security, in favor of admin server endpoints (/metrics
in this case), so I don't there is a need to report newly added metrics to mntr
. This also encourages users to migrate away from 4lw to admin end points.
Overall the state in current patch w.r.t this looks good to me.
From the conversation so far, it looks like we have reached a consensus regarding the directions of this patch and future metrics related patch. For this specific patch, it looks good to me, and I only have one comment regarding FOLLOWER_SYNC_TIME instrumentation but that's not blocking. Any comments from other reviewers? @anmolnar @eolivelli @lvfangmin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay to me
#shipit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 from me as well.
This patch adds several new server-side metrics as well as makes it easier to add new metrics in the future. This patch also includes a handful of other minor metrics-related changes. Here's a high-level summary of the changes. 1. This patch extends the request latency tracked in `ServerStats` to track `read` and `update` latency separately. Updates are any request that must be voted on and can change data, reads are all requests that can be handled locally and don't change data. 2. This patch adds the `ServerMetrics` logic and the related `AvgMinMaxCounter` and `SimpleCounter` classes. This code is designed to make it incredibly easy to add new metrics. To add a new metric you just add one line to `ServerMetrics` and then directly reference that new metric anywhere in the code base. The `ServerMetrics` logic handles creating the metric, properly adding the metric to the JSON output of the `/monitor` admin command, and properly resetting the metric when necessary. The motivation behind `ServerMetrics` is to make things easy enough that it encourages new metrics to be added liberally. Lack of in-depth metrics/visibility is a long-standing ZooKeeper weakness. At Facebook, most of our internal changes build on `ServerMetrics` and we have nearly 100 internal metrics at this time -- all of which we'll be upstreaming in the coming months as we publish more internal patches. 3. This patch adds 20 new metrics, 14 which are handled by `ServerMetrics`. 4. This patch replaces some uses of `synchronized` in `ServerStats` with atomic operations. Here's a list of new metrics added in this patch: - `uptime`: time that a peer has been in a stable leading/following/observing state - `leader_uptime`: uptime for peer in leading state - `global_sessions`: count of global sessions - `local_sessions`: count of local sessions - `quorum_size`: configured ensemble size - `synced_observers`: similar to existing `synced_followers` but for observers - `fsynctime`: time to fsync transaction log (avg/min/max) - `snapshottime`: time to write a snapshot (avg/min/max) - `dbinittime`: time to reload database -- read snapshot + apply transactions (avg/min/max) - `readlatency`: read request latency (avg/min/max) - `updatelatency`: update request latency (avg/min/max) - `propagation_latency`: end-to-end latency for updates, from proposal on leader to committed-to-datatree on a given host (avg/min/max) - `follower_sync_time`: time for follower to sync with leader (avg/min/max) - `election_time`: time between entering and leaving election (avg/min/max) - `looking_count`: number of transitions into looking state - `diff_count`: number of diff syncs performed - `snap_count`: number of snap syncs performed - `commit_count`: number of commits performed on leader - `connection_request_count`: number of incoming client connection requests - `bytes_received_count`: similar to existing `packets_received` but tracks bytes
Add `ServerMetricTests` test which tests the current `Metric` implementations `AvgMinMaxCounter` and `SimpleCounter`.
Remove unused local variable that was triggering `findbugs` warning.
0429b23
to
3c51230
Compare
Rebased to resolve merge conflict. |
Thanks for rebase, @jtuple. Latest build failed, but those are flaky tests. Kicking Jenkins again to get a green build. @anmolnar Could you please also sign this off? I believe you raised a request of removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No other concerns, thanks.+1
Got a green build. Merging. |
This patch adds several new server-side metrics as well as makes it easier to add new metrics in the future. This patch also includes a handful of other minor metrics-related changes. Here's a high-level summary of the changes. 1. This patch extends the request latency tracked in `ServerStats` to track `read` and `update` latency separately. Updates are any request that must be voted on and can change data, reads are all requests that can be handled locally and don't change data. 2. This patch adds the `ServerMetrics` logic and the related `AvgMinMaxCounter` and `SimpleCounter` classes. This code is designed to make it incredibly easy to add new metrics. To add a new metric you just add one line to `ServerMetrics` and then directly reference that new metric anywhere in the code base. The `ServerMetrics` logic handles creating the metric, properly adding the metric to the JSON output of the `/monitor` admin command, and properly resetting the metric when necessary. The motivation behind `ServerMetrics` is to make things easy enough that it encourages new metrics to be added liberally. Lack of in-depth metrics/visibility is a long-standing ZooKeeper weakness. At Facebook, most of our internal changes build on `ServerMetrics` and we have nearly 100 internal metrics at this time -- all of which we'll be upstreaming in the coming months as we publish more internal patches. 3. This patch adds 20 new metrics, 14 which are handled by `ServerMetrics`. 4. This patch replaces some uses of `synchronized` in `ServerStats` with atomic operations. Here's a list of new metrics added in this patch: - `uptime`: time that a peer has been in a stable leading/following/observing state - `leader_uptime`: uptime for peer in leading state - `global_sessions`: count of global sessions - `local_sessions`: count of local sessions - `quorum_size`: configured ensemble size - `synced_observers`: similar to existing `synced_followers` but for observers - `fsynctime`: time to fsync transaction log (avg/min/max) - `snapshottime`: time to write a snapshot (avg/min/max) - `dbinittime`: time to reload database -- read snapshot + apply transactions (avg/min/max) - `readlatency`: read request latency (avg/min/max) - `updatelatency`: update request latency (avg/min/max) - `propagation_latency`: end-to-end latency for updates, from proposal on leader to committed-to-datatree on a given host (avg/min/max) - `follower_sync_time`: time for follower to sync with leader (avg/min/max) - `election_time`: time between entering and leaving election (avg/min/max) - `looking_count`: number of transitions into looking state - `diff_count`: number of diff syncs performed - `snap_count`: number of snap syncs performed - `commit_count`: number of commits performed on leader - `connection_request_count`: number of incoming client connection requests - `bytes_received_count`: similar to existing `packets_received` but tracks bytes Author: Joseph Blomstedt <jdb@fb.com> Reviewers: Allan Lyu <fangmin@apache.org>, Andor Molnár <andor@apache.org>, Enrico Olivelli <eolivelli@gmail.com>, Michael Han <hanm@apache.org> Closes apache#580 from jtuple/ZOOKEEPER-3098
This patch adds several new server-side metrics as well as makes it easier to add new metrics in the future. This patch also includes a handful of other minor metrics-related changes.
Here's a high-level summary of the changes.
This patch extends the request latency tracked in
ServerStats
totrack
read
andupdate
latency separately. Updates are any requestthat must be voted on and can change data, reads are all requests that
can be handled locally and don't change data.
This patch adds the
ServerMetrics
logic and the relatedAvgMinMaxCounter
and
SimpleCounter
classes. This code is designed to make it incredibly easy toadd new metrics. To add a new metric you just add one line to
ServerMetrics
andthen directly reference that new metric anywhere in the code base. The
ServerMetrics
logic handles creating the metric, properly adding the metric to the JSON output of
the
/monitor
admin command, and properly resetting the metric when necessary.The motivation behind
ServerMetrics
is to make things easy enough that it encouragesnew metrics to be added liberally. Lack of in-depth metrics/visibility is a long-standing
ZooKeeper weakness. At Facebook, most of our internal changes build on
ServerMetrics
andwe have nearly 100 internal metrics at this time -- all of which we'll be upstreaming
in the coming months as we publish more internal patches.
This patch adds 20 new metrics, 14 which are handled by
ServerMetrics
.This patch replaces some uses of
synchronized
inServerStats
with atomic operations.Here's a list of new metrics added in this patch:
uptime
: time that a peer has been in a stable leading/following/observing stateleader_uptime
: uptime for peer in leading stateglobal_sessions
: count of global sessionslocal_sessions
: count of local sessionsquorum_size
: configured ensemble sizesynced_observers
: similar to existingsynced_followers
but for observersfsynctime
: time to fsync transaction log (avg/min/max)snapshottime
: time to write a snapshot (avg/min/max)dbinittime
: time to reload database -- read snapshot + apply transactions (avg/min/max)readlatency
: read request latency (avg/min/max)updatelatency
: update request latency (avg/min/max)propagation_latency
: end-to-end latency for updates, from proposal on leader to committed-to-datatree on a given host (avg/min/max)follower_sync_time
: time for follower to sync with leader (avg/min/max)election_time
: time between entering and leaving election (avg/min/max)looking_count
: number of transitions into looking statediff_count
: number of diff syncs performedsnap_count
: number of snap syncs performedcommit_count
: number of commits performed on leaderconnection_request_count
: number of incoming client connection requestsbytes_received_count
: similar to existingpackets_received
but tracks bytes