refactor(bigtable): move clone method from Metric to TableSchemaMetric - #16359
refactor(bigtable): move clone method from Metric to TableSchemaMetric#16359scotthart wants to merge 2 commits into
Conversation
product-auto-label
Bot
added
the
api: bigtable
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Bigtable metrics implementation by extracting table-specific schema metrics from metrics.cc and metrics.h into new dedicated files table_schema_metrics.cc and table_schema_metrics.h. It also introduces a MetricSchema enum to categorize metrics and updates the cloning logic to handle table schema metrics specifically. Feedback on the changes highlights several critical issues: a correctness bug in AttemptLatency2 where stale peer info labels are not reset on null peer info, a potential undefined behavior (invalid downcast) in operation_context_test.cc due to incorrect inheritance of MockMetric, violations of the repository style guide regarding the use of auto for protobuf messages and absl::string_view instead of std::string_view, and inconsistent/unsafe usage of .release() when initializing OpenTelemetry shared pointers.
| class MockMetric : public Metric { | ||
| public: | ||
| MetricSchema schema() const override { return MetricSchema::kTable; } |
There was a problem hiding this comment.
MockMetric in operation_context_test.cc inherits from Metric but overrides schema() to return MetricSchema::kTable. This violates the class hierarchy and triggers undefined behavior (invalid downcast) in CloneMetrics (defined in operation_context_factory.cc), which performs a static_cast<TableSchemaMetric const*> on any metric returning MetricSchema::kTable. MockMetric should inherit from TableSchemaMetric instead. Note that you will also need to include 'google/cloud/bigtable/internal/table_schema_metrics.h' at the top of this file.
class MockMetric : public TableSchemaMetric {
public:| #include <string> | ||
| #include <unordered_map> | ||
| #include <vector> |
There was a problem hiding this comment.
Add #include <string_view> to support the use of std::string_view in table_schema_metrics.cc.
#include <string>
#include <string_view>
#include <unordered_map>
#include <vector>There was a problem hiding this comment.
Better idea, #include <string_view> directly in table_schema_metrics.cc to reduce compile time wherever this .h file is included.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16359 +/- ##
=======================================
Coverage 92.23% 92.24%
=======================================
Files 2227 2229 +2
Lines 209573 209659 +86
=======================================
+ Hits 193309 193395 +86
Misses 16264 16264 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
was it intentional to move all of the tests? If so, delete the file?
| v2::MutateRowRequest const& request, auto const&) { | ||
| v2::MutateRowRequest const& request, | ||
| bigtable_internal::OperationContext&) { |
| clock_); | ||
| } | ||
|
|
||
| std::vector<std::shared_ptr<Metric>> CloneMetrics( |
There was a problem hiding this comment.
I looked at this and not much else.
2 participants
In preparation for adding client schema metrics, we're moving the
clonemethod fromMetrictoTableSchemaMetricas it's signature is specific to theLabelstype being passed. We're also taking this opportunity to reduce the size of the metrics* files by adding table_schema_metrics* files, with client_schema_metrics* files coming soon. To facilitate cloning, we introduced aschemamethod to indicate what the derived class is when processing aMetric. We could also have used a double dispatch pattern to achieve this, but opted to keep things simpler for the time being.