impl(bigtable): make MetricServiceConnection more shareable

[scotthart]

Refactor the MetricServiceConnection from inside the OperationContextFactory to DataConnectionImpl. This enables sharing of the MetricServiceConnection across exporters in the future.

[gemini-code-assist]

Code Review

This pull request integrates OpenTelemetry metrics into the Bigtable data connection by creating and passing a "MetricServiceConnection" to the "MetricsOperationContextFactory". The review feedback suggests several improvements to align with the style guide and optimize compilation: forward-declaring "MetricServiceConnection" in the header to reduce header pollution, wrapping helper functions in an anonymous namespace in the source file, and replacing default parameters with overloads for functions in the internal namespace.

[gemini-code-assist]

Instead of including "google/cloud/monitoring/v3/metric_connection.h" in the header file, we can use a forward declaration of MetricServiceConnection. This reduces header pollution and avoids unnecessary dependencies when GOOGLE_CLOUD_CPP_BIGTABLE_WITH_OTEL_METRICS is not defined.

namespace google::cloud::monitoring_v3 {
class MetricServiceConnection;
}  // namespace google::cloud::monitoring_v3

[gemini-code-assist]

Since we forward-declared MetricServiceConnection in the header, we should include "google/cloud/monitoring/v3/metric_connection.h" here inside the #ifdef GOOGLE_CLOUD_CPP_BIGTABLE_WITH_OTEL_METRICS block. Additionally, we should wrap MetricsExporterConnectionOptions in an anonymous namespace to give it internal linkage and avoid polluting the namespace, as recommended by the Google C++ Style Guide.

#ifdef GOOGLE_CLOUD_CPP_BIGTABLE_WITH_OTEL_METRICS
#include "google/cloud/monitoring/v3/metric_connection.h"

namespace {
Options MetricsExporterConnectionOptions(Options options) {
  // We start with a copy of the client options to preserve credentials and
  // universe domain, but we must unset Bigtable-specific endpoints/authorities
  // to allow default Monitoring defaults.
  options.unset<EndpointOption>();
  options.unset<AuthorityOption>();
  return options;
}
}  // namespace
#endif  // GOOGLE_CLOUD_CPP_BIGTABLE_WITH_OTEL_METRICS

References

1. Use of anonymous namespaces is encouraged for all code that does not need to be referenced from outside that file. Do not use static for functions or variables in namespace scope; use anonymous namespaces instead. ^(link)

[gemini-code-assist]

The repository style guide discourages default parameters for functions in an internal namespace. We should avoid using default parameters like Options options = {} for these constructors. Instead, we can provide overloads to maintain the same convenience without violating the style guide.

  MetricsOperationContextFactory(
      std::string client_uid,
      std::shared_ptr<monitoring_v3::MetricServiceConnection> conn)
      : MetricsOperationContextFactory(std::move(client_uid), std::move(conn),
                                       Options{}) {}

  MetricsOperationContextFactory(
      std::string client_uid,
      std::shared_ptr<monitoring_v3::MetricServiceConnection> conn,
      Options options);

  // Used for injecting a MockMetricsServiceConnection for testing.
  MetricsOperationContextFactory(
      std::string client_uid,
      std::shared_ptr<monitoring_v3::MetricServiceConnection> conn,
      std::shared_ptr<OperationContext::Clock> clock)
      : MetricsOperationContextFactory(std::move(client_uid), std::move(conn),
                                       std::move(clock), Options{}) {}

  MetricsOperationContextFactory(
      std::string client_uid,
      std::shared_ptr<monitoring_v3::MetricServiceConnection> conn,
      std::shared_ptr<OperationContext::Clock> clock, Options options);

References

1. Discourage default parameters for functions in an internal namespace. ^(link)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.24%. Comparing base (f1a8e57) to head (56a1bc4).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #16217   +/-   ##
=======================================
  Coverage   92.23%   92.24%           
=======================================
  Files        2265     2265           
  Lines      210205   210211    +6     
=======================================
+ Hits       193888   193904   +16     
+ Misses      16317    16307   -10     

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