feat(storage): add OTel metrics feature gate options and env vars by cpriti-os · Pull Request #16637 · googleapis/google-cloud-cpp

cpriti-os · 2026-11-09T06:07:04Z

Adds experimental options and environment variable overrides to gate OpenTelemetry client metrics collection for Google Cloud Storage:

storage_experimental::EnableOTelMetricsOption (overridden by GCP_STORAGE_CPP_ENABLE_OTEL_METRICS)
storage_experimental::EnableOTelDebugMetricsOption (overridden by GCP_STORAGE_CPP_ENABLE_OTEL_DEBUG_METRICS)

Precedence is environment variable ("true" / "false") > option in code > default (false). This PR introduces the configuration options and environment variable parsing; the metric instruments that read these options are wired in follow-up PRs.

Created by cpriti-o11y-agent

Add `storage_experimental::EnableOTelMetricsOption` and `storage_experimental::EnableOTelDebugMetricsOption` to configure OpenTelemetry client metrics and debug metrics collection for Storage, defaulting to `false`. Support `GCP_STORAGE_CPP_ENABLE_OTEL_METRICS` and `GCP_STORAGE_CPP_ENABLE_OTEL_DEBUG_METRICS` environment variable overrides (`"true"` / `"false"") in `DefaultOptions()`, consistent with the Go and Java Storage SDKs. [Generated-by: AI]

gemini-code-assist

Code Review

This pull request introduces EnableOTelMetricsOption and EnableOTelDebugMetricsOption to configure OpenTelemetry client metrics and debug metrics collection for Storage, allowing overrides via environment variables. Unit tests are also added to verify this behavior. The review feedback recommends explicitly specifying the return type std::optionalstd::string instead of using auto when retrieving environment variables, in order to comply with the repository's style guide against obscuring return types.

gemini-code-assist · 2026-11-09T06:11:37Z

+  if (!o.has<storage_experimental::EnableOTelMetricsOption>()) {
+    o.set<storage_experimental::EnableOTelMetricsOption>(false);
+  }
+  auto const otel_metrics_env = GetEnv("GCP_STORAGE_CPP_ENABLE_OTEL_METRICS");


According to the repository style guide, auto should not be used when it hides function return types. Please explicitly specify the return type std::optional<std::string> instead of using auto.

std::optional<std::string> const otel_metrics_env = GetEnv("GCP_STORAGE_CPP_ENABLE_OTEL_METRICS");

References

Reject Obscured Domain & Return Types: Flag and reject auto when it hides StatusOr<T>, domain objects, protobuf messages/fields, or function return types. ^(link)

Updated to use std::optional<std::string> const explicitly.

Replied by cpriti-o11y-agent

gemini-code-assist · 2026-11-09T06:11:37Z

+  auto const otel_debug_metrics_env =
+      GetEnv("GCP_STORAGE_CPP_ENABLE_OTEL_DEBUG_METRICS");


According to the repository style guide, auto should not be used when it hides function return types. Please explicitly specify the return type std::optional<std::string> instead of using auto.

std::optional<std::string> const otel_debug_metrics_env = GetEnv("GCP_STORAGE_CPP_ENABLE_OTEL_DEBUG_METRICS");

References

Reject Obscured Domain & Return Types: Flag and reject auto when it hides StatusOr<T>, domain objects, protobuf messages/fields, or function return types. ^(link)

Updated to use std::optional<std::string> const explicitly.

Replied by cpriti-o11y-agent

…aultOptions

codecov · 2026-11-09T10:06:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.45%. Comparing base (b1e9b10) to head (f62d677).
⚠️ Report is 27 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16637      +/-   ##
==========================================
- Coverage   92.48%   92.45%   -0.03%     
==========================================
  Files        2269     2269              
  Lines      222192   222270      +78     
==========================================
+ Hits       205487   205507      +20     
- Misses      16705    16763      +58

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

…tOptions complexity

kalragauri

nit: the description names the env vars GOOGLE_CLOUD_CPP_STORAGE_ENABLE_OTEL_METRICS / ..._DEBUG_METRICS, but the code uses GCP_STORAGE_CPP_ENABLE_OTEL_METRICS / ..._DEBUG_METRICS. Also, "consistent with Go and Java" only holds for Go: in Java the builder setting takes precedence over the env var. Could you fix the names, state the precedence explicitly, and note that nothing consumes these options yet?

kalragauri · 2026-11-13T08:45:15Z

 };

+/**
+ * Enable or disable OpenTelemetry client metrics collection for Storage.


EnableOTelMetricsOption is documented as enabling metrics collection, but nothing reads it yet, so setting it has no effect. Is that intentional?

Please also document how it will interact with EnableGrpcMetricsOption.

Yes, this is intentionally split out first to keep each PR in the series small and easy to review; the metric instruments that read these options are wired in the follow-up PRs. We also updated the docstrings here to clarify that both options are independent of EnableGrpcMetricsOption.

Replied by cpriti-o11y-agent

kalragauri · 2026-11-13T08:46:31Z

+  }
+  std::optional<std::string> const val = GetEnv(env_var);
+  if (!val.has_value()) return;
+  if (*val == "true") {


This accepts only the exact lowercase strings, while storage clients in other languages also accept 1, t, and any casing for the equivalent variables. A deployment that sets =1 across languages would silently get no metrics from C++.

Is the stricter parsing intentional, and if so, should unrecognized values log a warning?

Yes, matching only exact "true" and "false" and silently ignoring other or empty values is intentional to stay consistent with GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS_OVER_INTERCONNECT in default_options.cc.

Replied by cpriti-o11y-agent

kalragauri · 2026-11-13T08:47:17Z

+
+TEST_F(ClientTest, OTelMetricsEnvVarOverridesOption) {
+  {
+    ScopedEnvironment env("GCP_STORAGE_CPP_ENABLE_OTEL_METRICS", "true");


Both env vars and both options are always set to the same value in these tests, so they would still pass if DefaultOptions() read the debug variable for EnableOTelMetricsOption and vice versa.

Consider adding a case where only one variable is "true" and asserting the other option stays false.

Good catch—updated OTelMetricsDefaultAndOption and OTelMetricsEnvVarOverridesOption to test each option and environment variable independently with opposite boolean values.

Replied by cpriti-o11y-agent

kalragauri · 2026-11-13T08:47:46Z

+/**
+ * Enable or disable OpenTelemetry client metrics collection for Storage.
+ *
+ * The default is `false`. The `GCP_STORAGE_CPP_ENABLE_OTEL_METRICS`


GCP_STORAGE_CPP_ENABLE_OTEL_METRICS and the debug variant are only documented here. Could you also add them to google/cloud/storage/doc/environment-variables.dox, next to the other experimental variables?

Added both GCP_STORAGE_CPP_ENABLE_OTEL_METRICS and GCP_STORAGE_CPP_ENABLE_OTEL_DEBUG_METRICS to google/cloud/storage/doc/environment-variables.dox.

Replied by cpriti-o11y-agent

kalragauri · 2026-11-13T08:51:25Z

+}
+
+TEST_F(ClientTest, OTelMetricsInvalidEnvVarIgnored) {
+  for (char const* val : {"1", "0", "TRUE", "False", "ON", "yes", ""}) {


A failure inside these nested loops would not say which val or configured combination failed.

Can we replace this with TEST_P and ::testing::Combine, so each case is reported by name?

Added SCOPED_TRACE with val and configured inside the loop so any assertion failure reports the exact combination without the extra boilerplate of a TEST_P fixture.

Replied by cpriti-o11y-agent

cpriti-os · 2026-11-13T09:14:20Z

Updated the PR description with the exact GCP_STORAGE_CPP_ENABLE_OTEL_METRICS and GCP_STORAGE_CPP_ENABLE_OTEL_DEBUG_METRICS names, precedence order (environment variable > option in code > default), and note on the follow-up PRs.

Replied by cpriti-o11y-agent

product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Nov 9, 2026

gemini-code-assist Bot reviewed Nov 9, 2026

View reviewed changes

cpriti-os marked this pull request as ready for review November 9, 2026 06:19

cpriti-os requested review from a team as code owners November 9, 2026 06:19

cpriti-os requested a review from kalragauri November 9, 2026 06:19

cpriti-os added 2 commits November 9, 2026 07:04

fix(storage): avoid range-for string copy in client_test

20035b1

style(storage): use explicit types instead of auto for GetEnv and Def…

4fcc9b0

…aultOptions

refactor(storage): extract ApplyBoolEnvOption helper to reduce Defaul…

e024623

…tOptions complexity

kalragauri reviewed Nov 13, 2026

View reviewed changes

chore(storage): address review comments on OTel metrics options

f62d677

		auto const otel_debug_metrics_env =
		GetEnv("GCP_STORAGE_CPP_ENABLE_OTEL_DEBUG_METRICS");

Conversation

cpriti-os commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Nov 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kalragauri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpriti-os commented Nov 13, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cpriti-os commented Nov 9, 2026 •

edited

Loading

codecov Bot commented Nov 9, 2026 •

edited

Loading