Re-implement connected_client to have deterministic cardinality

[mindw]

The current connected client suffers from some drawbacks rendering much less useful than it should:

1. unbound cardinality - the labels "idle_since", "omem" and "cmd" values are unpredictable and there for unbound.
2. Having values as labels made using them in queries all but impossible, getting the idlest client for example.
3. The id field which uniquely identifies the client is missing from the labels.

This PR attempts to ratify all that:

• renames connected_clients_details to connected_client_info (as per prometheus naming conventions) - dropping all high cardinality labels.
• adds connected_client_output_buffer_memory_usage_bytes, connected_client_total_memory_consumed, connected_client_created_at_timestamp, connected_client_idle_since_timestamp, connected_client_channel_subscriptions_count, connected_client_pattern_matching_subscriptions_count, connected_client_shard_channel_subscriptions_count and connected_client_shard_channel_watched_keys metrics per client.
• add id to all metrics.

I'm still running some tests on the output metrics, so please don't merge it yet :)

Input, missing bits (docs?) are welcome!

[hiroshi-ishii]

Quick comments (warning: I do not know any go):

1. Did you intend to remove the parsing of Cmd entirely (i.e. not just removing it as a label)?
2. Can you also parse/output the following: qbuf, qbuf-free, obl, oll
3. port could be an int64; it may be useful to do numerical comparisons on it
4. Typo on several lines (e.g. 63): cloud not parse ...; not yours but might as well fix it
5. line 102: error message for watch case instead prints ... 'sub' field ...
6. Can we replace connected_client_info with flags as a label (there are many flags that can change moment-to-moment), with connected_client_flags as flags as the gauge value? Not sure about this one.
7. Don't forget to remove the log.Debug() in the ExportClientsInclPort case
8. Should metric description's descriptions start with caps? i.e. A connected client's ...
9. I think name should not be part of clientBaseLabels since name can be changed with CLIENT SETNAME. The name should be mapped to the id by other means.
10. Could we refactor metricDescriptions / registerConstMetricGauge calls with an array & loop -- make it shorter and easier to read?

[mindw]

Thanks @hiroshi-ishii for the review - see below:

Quick comments (warning: I do not know any go):

1. Did you intend to remove the parsing of Cmd entirely (i.e. not just removing it as a label)?
   It was - are there leftovers?
2. Can you also parse/output the following: qbuf, qbuf-free, obl, oll
   Yes, I'll add it later.
3. port could be an int64; it may be useful to do numerical comparisons on it
   Port is arbitrary, and isn't unique, I don't recall ever it being exposed as a value.
4. Typo on several lines (e.g. 63): cloud not parse ...; not yours but might as well fix it
   Fixed.
5. line 102: error message for watch case instead prints ... 'sub' field ...
   Fixed.
6. Can we replace connected_client_info with flags as a label (there are many flags that can change moment-to-moment), with connected_client_flags as flags as the gauge value? Not sure about this one.
   Possible - need to look into this.
7. Don't forget to remove the log.Debug() in the ExportClientsInclPort case
   Fixed, thanks!
8. Should metric description's descriptions start with caps? i.e. A connected client's ...
   Yes, fixed.
9. I think name should not be part of clientBaseLabels since name can be changed with CLIENT SETNAME. The name should be mapped to the id by other means.
   That's actually the reason for it to be on the info metric. While ID is unique and name is not, the name is far more human friendly IMO.
10. Could we refactor metricDescriptions / registerConstMetricGauge calls with an array & loop -- make it shorter and easier to read?
    Maybe, from my experience these kind of generics hurt readability and maintainability. In case of exporters it is quite common to avoid them unless there is something smarter behind them (like generating metrics from struct members dynamically to support future changes).

[oliver006]

Hey there and thanks for opening the PR!
I haven't looked at the code yet but I like the idea a lot to break out the details into separate metrics that can be queried and graphed. So go ahead with that.

One request though: can we keep the name of the current client detail metric the same so you don't introduce backwards incompatible changes?

Let me know when you're ready for code review and want comments and then I'll have a look.

Oh, and one more request: can you rebase the PR please? I moved the tests over to Github Actions yesterday and I don't see that here yet.

[mindw]

Hey there and thanks for opening the PR! I haven't looked at the code yet but I like the idea a lot to break out the details into separate metrics that can be queried and graphed. So go ahead with that.

Tx!

One request though: can we keep the name of the current client detail metric the same so you don't introduce backwards incompatible changes?

It is an optional feature and I'm not sure how much it was used given it's current state also it wont have the same labels. Then again the old name doesn't really follow the Prometheus naming convention. I would suggest to duplicate it the name instead.

Let me know when you're ready for code review and want comments and then I'll have a look.

It's ready :)

Oh, and one more request: can you rebase the PR please? I moved the tests over to Github Actions yesterday and I don't see that here yet.

I've rebased on the latest and greatest main/master.

Thanks!

[hiroshi-ishii]

@mindw
On (2) Thanks!
On (3) I didn't understand your response.
On (6) maybe we could replace flags in the labels with a subset of mostly permanent flags (e.g. client is replica, client is master)
On (9), on second thought, you're right, and client name doesn't change that often so we're OK.
On (10) I understand what you're saying but I still think it's worthwhile in this case. But it's your rodeo...

[hiroshi-ishii]

Looked at the new changes

[hiroshi-ishii]

Thanks @mindw, looks good.

Let's see what @oliver006 says.

[coveralls]

Pull Request Test Coverage Report for Build 10993316969

Details

• 183 of 183 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.0%) to 85.116%

---

Totals
Change from base Build 10969721321:	1.0%
Covered Lines:	2173
Relevant Lines:	2553

---

💛 - Coveralls

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.63%. Comparing base (827e6b5) to head (4ff2795).
Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #950      +/-   ##
==========================================
+ Coverage   80.25%   81.63%   +1.38%     
==========================================
  Files          17       17              
  Lines        1980     2129     +149     
==========================================
+ Hits         1589     1738     +149     
  Misses        306      306              
  Partials       85       85              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mindw]

@oliver006 this is ready for your review :)

[oliver006]

@oliver006 this is ready for your review :)

Sorry about the delay - having a look now.

[oliver006]

Overall approach looks good - just a couple of smaller things

[mindw]

@oliver006 IMO all comments were addressed, anything else?

[oliver006]

@oliver006 IMO all comments were addressed, anything else?

Apologies for the delay, I didn't have much time last week, will get back to this again in the next few days.

[oliver006]

Nice work - thanks for getting this done!