wip

[scotthart]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces a new OutstandingRpcs metric to track the instantaneous count of outstanding RPCs on selected channels within dynamic channel pools, updating the channel pool, decorators, and metrics exporter accordingly. The review feedback highlights several critical safety issues, specifically potential null pointer dereferences in bigtable_random_two_least_used_decorator.cc and operation_context.cc that could cause crashes. Additionally, the reviewer recommended optimizing string copies by using std::move in grpc_metrics_exporter.cc and simplifying a complex std::set_difference operation in metrics.cc with a more readable loop, aligning with the repository's style guide.

[gemini-code-assist]

The operation_context parameter is a std::shared_ptr and can be nullptr (for example, when metrics are disabled or in certain unit tests). Dereferencing it directly without a null check will cause a crash. Please add a null check before calling StubSelection.

template <typename Response>
Response AsyncHelper(std::shared_ptr<DynamicChannelPool<BigtableStub>>& pool,
                     std::shared_ptr<OperationContext> const& operation_context,
                     std::function<Response(BigtableStub&)> fn) {
  SelectedChannel<BigtableStub> selection =
      pool->GetChannelRandomTwoLeastUsed();
  if (operation_context) {
    operation_context->StubSelection(StubSelectionParams{
        selection.outstanding_rpcs, ChannelPoolLbPolicy::kRandomTwoLeastUsed,
        pool->transport_type(), RpcType::kUnary});
  }
  std::shared_ptr<BigtableStub> stub = selection.channel->AcquireStub();
  Response result = fn(*stub);
  selection.channel->ReleaseStub();
  return result;
}

[gemini-code-assist]

The operation_context parameter is a std::shared_ptr and can be nullptr. Dereferencing it directly without a null check will cause a crash. Please add a null check before calling StubSelection.

template <typename Response>
std::unique_ptr<internal::StreamingReadRpc<Response>> StreamingHelper(
    std::shared_ptr<DynamicChannelPool<BigtableStub>>& pool,
    std::shared_ptr<OperationContext> const& operation_context,
    std::function<std::unique_ptr<internal::StreamingReadRpc<Response>>(
        BigtableStub&)>
        fn) {
  SelectedChannel<BigtableStub> selection =
      pool->GetChannelRandomTwoLeastUsed();
  if (operation_context) {
    operation_context->StubSelection(StubSelectionParams{
        selection.outstanding_rpcs, ChannelPoolLbPolicy::kRandomTwoLeastUsed,
        pool->transport_type(), RpcType::kStreaming});
  }
  std::shared_ptr<BigtableStub> stub = selection.channel->AcquireStub();
  std::unique_ptr<internal::StreamingReadRpc<Response>> result = fn(*stub);
  auto release_fn = [weak = selection.channel->MakeWeak()] {
    std::shared_ptr<ChannelUsage<BigtableStub>> child = weak.lock();
    if (child) child->ReleaseStub();
  };
  return std::make_unique<StreamingReadRpcTracking<Response>>(
      std::move(result), std::move(release_fn));
}

[gemini-code-assist]

The operation_context parameter is a std::shared_ptr and can be nullptr. Dereferencing it directly without a null check will cause a crash. Please add a null check before calling StubSelection.

template <typename Response>
std::unique_ptr<internal::AsyncStreamingReadRpc<Response>> AsyncStreamingHelper(
    std::shared_ptr<DynamicChannelPool<BigtableStub>>& pool,
    std::shared_ptr<OperationContext> const& operation_context,
    std::function<std::unique_ptr<internal::AsyncStreamingReadRpc<Response>>(
        BigtableStub&)>
        fn) {
  SelectedChannel<BigtableStub> selection =
      pool->GetChannelRandomTwoLeastUsed();
  if (operation_context) {
    operation_context->StubSelection(StubSelectionParams{
        selection.outstanding_rpcs, ChannelPoolLbPolicy::kRandomTwoLeastUsed,
        pool->transport_type(), RpcType::kStreaming});
  }
  std::shared_ptr<BigtableStub> stub = selection.channel->AcquireStub();
  std::unique_ptr<internal::AsyncStreamingReadRpc<Response>> result = fn(*stub);
  auto release_fn = [weak = selection.channel->MakeWeak()] {
    std::shared_ptr<ChannelUsage<BigtableStub>> child = weak.lock();
    if (child) child->ReleaseStub();
  };
  return std::make_unique<AsyncStreamingReadRpcTracking<Response>>(
      std::move(result), std::move(release_fn));
}

[gemini-code-assist]

The operation_context parameter is a std::shared_ptr and can be nullptr. Dereferencing it directly without a null check will cause a crash. Please add a null check before calling StubSelection.

template <typename Request, typename Response>
std::unique_ptr<AsyncStreamingReadWriteRpc<Request, Response>>
AsyncStreamingHelper(
    std::shared_ptr<DynamicChannelPool<BigtableStub>>& pool,
    std::shared_ptr<OperationContext> const& operation_context,
    std::function<
        std::unique_ptr<AsyncStreamingReadWriteRpc<Request, Response>>(
            BigtableStub&)>
        fn) {
  SelectedChannel<BigtableStub> selection =
      pool->GetChannelRandomTwoLeastUsed();
  if (operation_context) {
    operation_context->StubSelection(StubSelectionParams{
        selection.outstanding_rpcs, ChannelPoolLbPolicy::kRandomTwoLeastUsed,
        pool->transport_type(), RpcType::kStreaming});
  }
  std::shared_ptr<BigtableStub> stub = selection.channel->AcquireStub();
  std::unique_ptr<AsyncStreamingReadWriteRpc<Request, Response>> result =
      fn(*stub);
  auto release_fn = [weak = selection.channel->MakeWeak()] {
    std::shared_ptr<ChannelUsage<BigtableStub>> child = weak.lock();
    if (child) child->ReleaseStub();
  };

[gemini-code-assist]

This block uses std::set_difference with a custom local comparator struct to filter out keys from a map that are present in a set. This is overly complex and represents 'Code Poetry'. We can replace this entire block with a simple, readable, and idiomatic loop.

  for (auto const& kv : data) {
    if (filtered_data_labels.find(kv.first) == filtered_data_labels.end()) {
      labels.emplace(kv.first, kv.second);
    }
  }
  return labels;

References

1. Dismantle complex abstractions used for simple tasks. Prefer simplicity over cleverness. ^(link)

[gemini-code-assist]

The local variable project_id is returned as part of MonitoredResourceResult which is constructed inline. Since NRVO does not apply here, not moving project_id results in an unnecessary copy of the string. Please use std::move to avoid this copy.

  return MonitoredResourceResult{std::move(project_id), std::move(resource)};

References

1. Scrutinize copies of non-fundamental C++ types. Is it necessary to copy the data? Can we move the data instead? ^(link)

[gemini-code-assist]

If any element in the metrics vector is nullptr, calling m->clone will result in a null pointer dereference and crash. Please add a null check for m before cloning.

  for (auto const& m : metrics) {
    if (!m) continue;
    auto clone = m->clone(resource_labels, data_labels);
    if (!clone) {
      clone = m->clone(client_resource_labels);
    }
    if (clone) {
      v.emplace_back(std::move(clone));
    }
  }

[codecov]

Codecov Report

❌ Patch coverage is 73.84045% with 141 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.24%. Comparing base (2f177ae) to head (6eec3ae).

Files with missing lines	Patch %	Lines
...d/bigtable/tests/observability_integration_test.cc	3.70%	104 Missing ⚠️
...oud/bigtable/internal/operation_context_factory.cc	80.48%	16 Missing ⚠️
google/cloud/bigtable/internal/metrics.cc	85.85%	14 Missing ⚠️
...oud/bigtable/internal/data_connection_impl_test.cc	95.89%	3 Missing ⚠️
...gle/cloud/bigtable/internal/dynamic_channel_pool.h	91.30%	2 Missing ⚠️
google/cloud/bigtable/internal/metrics.h	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16350      +/-   ##
==========================================
- Coverage   92.24%   92.24%   -0.01%     
==========================================
  Files        2227     2227              
  Lines      209551   209869     +318     
==========================================
+ Hits       193293   193585     +292     
- Misses      16258    16284      +26     

☔ 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.