channel pool metric prototype - #16324
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces tracking for outstanding RPCs on selected channels within the Bigtable client's connection pool. It updates the BigtableStub interface and its decorators to accept an OperationContext parameter across all synchronous and asynchronous RPC methods. Additionally, it renames ResourceLabels and DataLabels to TableResourceLabels and TableDataLabels respectively, while introducing ClientResourceLabels and the OutstandingRpcs metric. Feedback on the changes suggests simplifying the label filtering logic in metrics.cc by replacing a complex std::set_difference call with a simpler loop, adhering to the repository's "Reject 'Code Poetry'" style guide principle.
| struct Compare { | ||
| bool operator()(std::pair<std::string const, std::string> const& a, | ||
| std::string const& b) { | ||
| return a.first < b; | ||
| } | ||
|
|
||
| bool operator()(std::string const& a, | ||
| std::pair<std::string const, std::string> const& b) { | ||
| return a < b.first; | ||
| } | ||
| }; | ||
|
|
||
| std::set_difference(data.begin(), data.end(), filtered_data_labels.begin(), | ||
| filtered_data_labels.end(), | ||
| std::inserter(labels, labels.begin()), Compare()); |
There was a problem hiding this comment.
Reject "Code Poetry": Simplify Label Filtering
The data map contains exactly 3 elements. Using std::set_difference with a custom Compare struct is overly complex for filtering such a small number of elements.
Per the repository style guide (Reject "Code Poetry"), we should prefer simplicity over cleverness. A simple loop checking if each key is in filtered_data_labels is much easier to read, maintain, and is likely more performant by avoiding the overhead of std::set_difference and custom comparators.
for (auto const& kv : data) {
if (filtered_data_labels.find(kv.first) == filtered_data_labels.end()) {
labels.insert(kv);
}
}References
- Reject "Code Poetry": Dismantle complex abstractions used for simple tasks. Prefer simplicity over cleverness. (link)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16324 +/- ##
==========================================
+ Coverage 92.24% 92.27% +0.03%
==========================================
Files 2227 2227
Lines 209543 209711 +168
==========================================
+ Hits 193284 193516 +232
+ Misses 16259 16195 -64 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
79de35f to
e0f403a
Compare
e0f403a to
568f9f5
Compare
568f9f5 to
7587a31
Compare
No description provided.