impl(bigtable): add directpath metric - #16370
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the DirectAccessCompatibility metric and its associated labels to track Bigtable DirectPath compatibility, and updates MakeClientResourceLabels to extract instance and application profile details from options. Unit tests are also added to verify these changes. The review feedback recommends a performance optimization in IntoLabelMap to avoid unnecessary string copies and allocations on the hot path by directly emplacing labels instead of using a temporary array of structs.
| struct { | ||
| std::string key; | ||
| std::string value; | ||
| } data[] = { | ||
| {"ip_preference", d.ip_preference}, | ||
| {"reason", d.reason}, | ||
| }; | ||
|
|
||
| for (auto& [key, value] : data) { | ||
| if (filtered_data_labels.find(key) == filtered_data_labels.end()) { | ||
| labels.emplace(std::move(key), std::move(value)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Creating a temporary array of structs with std::string members causes unnecessary copies and allocations of d.ip_preference and d.reason on every call to IntoLabelMap, which is on the hot path of recording metrics. Since there are only two labels to process, we can avoid the temporary array, the loop, and any unnecessary string copies by directly checking filtered_data_labels and emplacing the values.
if (filtered_data_labels.find("ip_preference") == filtered_data_labels.end()) {
labels.emplace("ip_preference", d.ip_preference);
}
if (filtered_data_labels.find("reason") == filtered_data_labels.end()) {
labels.emplace("reason", d.reason);
}References
- Scrutinize copies of non-fundamental C++ types. Is it necessary to copy the data? Can we move the data instead? (link)
- When using a map with transparent lookups (e.g.,
absl::flat_hash_map<std::string, ...>), preferfind()overoperator[]to avoid constructing a key object on every call. This is often more performant, especially if cache hits are common.
There was a problem hiding this comment.
Creation of temp structs removed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16370 +/- ##
==========================================
+ Coverage 92.26% 92.27% +0.01%
==========================================
Files 2239 2239
Lines 210719 210816 +97
==========================================
+ Hits 194412 194535 +123
+ Misses 16307 16281 -26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| LabelMap IntoLabelMap(ClientResourceLabels const& r, | ||
| DirectAccessCompatibilityLabels const& d, | ||
| std::set<std::string> const& filtered_data_labels = {}); |
There was a problem hiding this comment.
nit: avoid default params for internal functions
There was a problem hiding this comment.
Done. A follow-up PR will update the .md files to help catch and prevent this type of style violation from occurring.
| auto emplace_if_not_filtered = [&](std::string key, std::string value) { | ||
| if (filtered_data_labels.find(key) == filtered_data_labels.end()) { | ||
| labels.emplace(std::move(key), std::move(value)); |
There was a problem hiding this comment.
optional super nit:
I think if filtered_data_labels is a std::set<std::string, std::less<>> then we can have this fn take std::string_views as arguments, and only allocate the std::string if we need to emplace after the lookup.
This PR adds the client schema metric used to report the result of attempting to use DirectPath.