Skip to content

refactor(bigtable): decouple metric cloning from OperationContext - #16355

Open
scotthart wants to merge 2 commits into
googleapis:mainfrom
scotthart:bigtable_metrics_refactor_cloning
Open

refactor(bigtable): decouple metric cloning from OperationContext#16355
scotthart wants to merge 2 commits into
googleapis:mainfrom
scotthart:bigtable_metrics_refactor_cloning

Conversation

@scotthart

Copy link
Copy Markdown
Member

This PR decouples ResourceLabels and DataLabels from the OperationContext constructor. This increases the cohesiveness and genericity of the OperationContext class. This will be useful for adding Client Schema metrics to Bigtable and required for metrics support across other libraries.

@scotthart
scotthart requested a review from a team as a code owner August 18, 2026 16:05
@product-auto-label product-auto-label Bot added the api: bigtable Issues related to the Bigtable API. label Aug 18, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors metric cloning by moving the cloning logic out of OperationContext and introducing a standalone CloneMetrics helper function. This simplifies the OperationContext constructor to directly accept a vector of already-cloned metrics, and allows the removal of the CloningMetric test helper class across multiple test files. Feedback on the changes suggests replacing the assert in CloneMetrics with an explicit nullptr check to prevent potential null pointer dereferences in release builds where assertions are disabled.

Comment on lines +554 to +559
for (auto const& m : metrics) {
// We should never add a nullptr Metric to the list.
assert(m != nullptr);
auto clone = m->clone(resource_labels, data_labels);
v.emplace_back(std::move(clone));
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

In release builds, assert is compiled out, which means a null pointer in metrics would lead to a null pointer dereference and undefined behavior (crash) when calling m->clone(...). To ensure robustness and adhere to defensive programming practices, we should explicitly check for nullptr before dereferencing.

  for (auto const& m : metrics) {
    if (m == nullptr) continue;
    v.emplace_back(m->clone(resource_labels, data_labels));
  }
References
  1. Prefer defensive code, such as explicit ok() checks, even if they seem redundant based on the current implementation of a framework, as the framework's contract may change in the future.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@dbolduc dbolduc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the bot

@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.43503% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.23%. Comparing base (ac64c89) to head (6eef5c6).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
google/cloud/bigtable/internal/metrics.cc 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16355      +/-   ##
==========================================
- Coverage   92.24%   92.23%   -0.01%     
==========================================
  Files        2227     2227              
  Lines      209594   209531      -63     
==========================================
- Hits       193335   193268      -67     
- Misses      16259    16263       +4     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@scotthart
scotthart enabled auto-merge (squash) August 18, 2026 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants