Enhance the client phone home metrics #18326
Conversation
Formerly, we were just sending the active clients count at the time of the phone home operation. This is an attempt to enhance the metrics sent during the phone home. Now, along with the active clients count at the time of the phone home operation, we are also sending the following information: - Connections opened in the last 24 hours (connections authenticated) - Connections closed in the last 24 hours - Total connection duration in the last 24 hours (sum of the duration of the closed connections + active connections) - Client versions connected in the last 24 hours Now, during the phone home, each node will send the number of active clients in the cluster plus the local informations listed above. Note that, If the phone home is not enabled, we will not collect the informations listed above.
mdumandag
added
Type: Enhancement
Team: Client
Source: Internal
mdumandag
force-pushed
the
client-phone-homes
branch
from
86f56ce
to
c5624eb
Compare
| ClientEndpointStatistics stat = getStats(endpoint); | ||
| long uptime = getUptime(now, endpoint); | ||
| if (uptime > 0) { | ||
| stat.incrementTotalConnectionDuration(uptime); |
There was a problem hiding this comment.
Does it make sense to keep it in-memory and increment? We could also set it to now - creationTime and that would have the same result (unless I overlook something).
There was a problem hiding this comment.
If we were reporting all-time total connection duration, now-creationTime would work. But, we planned to send stats in 24-hour intervals and reset them afterward. Do you think that we should store and report all-time records instead?
I think this way, it is much easier to query, you just sum up values on a certain time interval. If we were reporting all-time values, queries have to sum up values reported recently by the unique nodes.
| * active at that time. | ||
| * @return map of client type to statistics snapshots. | ||
| */ | ||
| Map<String, ClientEndpointStatisticsSnapshot> getSnapshotsAndReset(Collection<ClientEndpoint> activeEndpoints); |
There was a problem hiding this comment.
nit: this could be called drainSnapshots()
There was a problem hiding this comment.
If it is okay, I would like to keep this and the method name on the Counter class as they are
| * @param newValue the new value. | ||
| * @return the old value of the counter. | ||
| */ | ||
| long getAndSet(long newValue); |
There was a problem hiding this comment.
nit: similarly, this could be drain() without parameters (the actual newValue is always 0 anyway)
There was a problem hiding this comment.
I thought this method might be useful in the future, we use counters in other places too. So, I tried to be as generic as possible here
There was a problem hiding this comment.
Sure, no prob, let's get this merged then.
Formerly, we were just sending the active clients count at the time
of the phone home operation. This is an attempt to enhance the metrics
sent during the phone home. Now, along with the active clients count
at the time of the phone home operation, we are also sending the following
information:
closed connections + active connections)
Now, during the phone home, each node will send the number of active clients
in the cluster plus the local informations listed above.
Note that, If the phone home is not enabled, we will not collect the informations
listed above.
Closes #18308