Add new metrics exporting several cluster tables #3790
Conversation
- Added new helper function 'p_update_map_gauge' - Improved helpers definitions using references.
|
Retest this please. |
| @@ -90,10 +90,25 @@ struct p_admin_gauge { | |||
| }; | |||
| }; | |||
|
|
|||
| struct p_admin_dyn_counter { | |||
There was a problem hiding this comment.
These might be written simpler as an enum class, enum class p_admin_dyn_counter { __size }} and still be scoped and accessible as p_admin_dyn_counter::__size;
There was a problem hiding this comment.
Yes, indeed we thought about that, but for this case we wanted the scoping but we didn't wanted to lose the automatic conversion from enum to int, that the why this was written in this way.
|
|
||
| // proxysql_servers_checksum | ||
| std::map<std::string, prometheus::Counter*> p_proxysql_servers_checksum_version {}; | ||
| std::map<std::string, prometheus::Gauge*> p_proxysql_servers_checksums_epoch {}; |
There was a problem hiding this comment.
While functional, some of these declarations and function parameter types could be shortened with type aliasing.
There was a problem hiding this comment.
yeah... but if we are going to make a convention we should make it in a later PR. I'm saying this just because the codebase has already lots of definitions like these ones. So, in a later PR we can propose shorten all of them, even if for these particular case I don't see much trouble, I like more the shortening of names when they need to be mentioned multiple times, but I like to keep the definitions easy to read.
| @@ -632,8 +633,26 @@ using admin_gauge_tuple = | |||
| metric_tags | |||
| >; | |||
|
|
|||
| using admin_dyn_counter_tuple = | |||
| std::tuple< | |||
There was a problem hiding this comment.
This maybe personal preference, but I would think that these instead could be made into a base type, as most of the members are the same. Perhaps a template struct, which takes an enum as the template parameter for the first member and constructor. Then for these other using declarations, a single line type alias would suffice like using admin_dyn_counter = metric_t<p_admin_dyn_counter>. This would also make compiler error messages easier to understand than if the make_tuple call was incorrect. Then, rather than std::get with index, access by member name, easier to read and less error prone if the data structure changed. For example, if another value was added to the tuple as the first param, would invalidate the indices used by std::get calls. Whereas, member access would not have to be rewritten. While the implementation here appears functional, it might not be necessary to refactor or redo this, however should be kept in mind for future implementations.
There was a problem hiding this comment.
I agree that a simple templated struct may do the job of std::tuple in this scenario, still the use of it right now is very simple and we do it like this in a lot of places already, also the API is very homogeneous and because of that mistakes are hard to make, still, wouldn't discard of course revisit it in the future offering a more robust solution 👍 .
|
retest this please |
2 similar comments
|
retest this please |
|
retest this please |
|
Automated message: PR pending admin approval for build testing |
Adds new metrics for: