zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jtuple <...@git.apache.org>
Subject [GitHub] zookeeper pull request #580: ZOOKEEPER-3098: Add additional server metrics
Date Fri, 07 Sep 2018 18:32:37 GMT
Github user jtuple commented on a diff in the pull request:

    https://github.com/apache/zookeeper/pull/580#discussion_r216049921
  
    --- Diff: src/java/main/org/apache/zookeeper/server/ServerStats.java ---
    @@ -33,17 +34,17 @@
     public class ServerStats {
         private static final Logger LOG = LoggerFactory.getLogger(ServerStats.class);
     
    -    private long packetsSent;
    -    private long packetsReceived;
    -    private long maxLatency;
    -    private long minLatency = Long.MAX_VALUE;
    -    private long totalLatency = 0;
    -    private long count = 0;
    +    private final AtomicLong packetsSent = new AtomicLong();
    --- End diff --
    
    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`.


---

Mime
View raw message