feat(otel): control tracing with environment variable #11897

dbolduc · 2023-06-14T23:34:33Z

Part of the work for #12316

Add a GOOGLE_CLOUD_CPP_OPENTELEMETRY_TRACING environment variable. I do not think we need EXPERIMENTAL in the name when it touches an experimental:: option.

~~Note that this option violates the "Environment variables take precedence" ADR. We have a reason to. I think it is fine as long as we communicate the expected behavior.~~

Touch up the documentation for the tracing option.

This change is

codecov · 2023-06-15T01:41:35Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (9357ce2) 93.61% compared to head (2d7c0e2) 93.61%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #11897   +/-   ##
=======================================
  Coverage   93.61%   93.61%           
=======================================
  Files        2029     2029           
  Lines      179715   179728   +13     
=======================================
+ Hits       168241   168257   +16     
+ Misses      11474    11471    -3

Files Changed	Coverage Δ
google/cloud/internal/populate_common_options.cc	`100.00% <100.00%> (ø)`
...gle/cloud/internal/populate_common_options_test.cc	`100.00% <100.00%> (ø)`

... and 4 files with indirect coverage changes

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

coryan · 2023-07-21T13:48:39Z

Should we keep this PR open?

coryan · 2023-08-08T18:01:09Z

google/cloud/internal/populate_common_options.cc

@@ -58,8 +59,16 @@ Options PopulateCommonOptions(Options opts, std::string const& endpoint_env_var,
    opts.set<UserProjectOption>(*std::move(e));
  }

+  if (!opts.has<experimental::OpenTelemetryTracingOption>()) {
+    // The Option takes precedence over the environment variable, so we can
+    // explicitly turn off tracing in the Cloud Trace client that implements our


Is that a configuration that customers will want to use? Meaning: "I have turned on tracing via an environment variable, but I also explicitly turned off exporting via code"? Or is this just something we need for our testing?

Do we need two separate options, one to turn off exporting and one to turn off tracing?

Is that a configuration that customers will want to use? Meaning: "I have turned on tracing via an environment variable, but I also explicitly turned off exporting via code"? Or is this just something we need for our testing?

It is really for us. I would not say it is for our testing. It is not really intended as a customer feature, although the crafty ones may use it.

It is working around the fact that our Cloud Trace client is no different from a Cloud Foo client.

Alternatively, we could add something like an internal::OverrideOpenTelemetryTracingOption that is used only by the exporter to accomplish the same thing. Something about an internal only options seems fishy though. Maybe it is justified in this case.

I do not think there is a good alternative that adds knobs to the generator.

Do we need two separate options, one to turn off exporting and one to turn off tracing?

No. They are separate. Exporting is configured through google_cloud_cpp_opentelemetry / opentelemetry-cpp. It is not done via google_cloud_cpp_common.

It is working around the fact that our Cloud Trace client is no different from a Cloud Foo client.

Ah, so we don't want to trace the trace exports? Maybe we can convince ourselves that it is Okay to do so:

Inside Google we trace the exports, and things do not blow up.

It helps to get a small sample of "the traces are exporting" traces.

Typically traces are sampled, so the trace exports are rare.

Typically traces are batched, so there is no explosion due to "tracing the exports creates even more traces!".

FTR, I did experiment with tracing the exporter, and it only creates 1 extra span.

I wasn't sure if other certain settings might lead to the recursive explosion, though.

I think we should keep this simple and trace the exporter. If it becomes a problem we can create some ad-hoc thing. Thoughts?

That works for me. I'll simplify this PR.

devbww · 2023-08-09T14:40:22Z

google/cloud/internal/populate_common_options.cc

@@ -58,8 +59,11 @@ Options PopulateCommonOptions(Options opts, std::string const& endpoint_env_var,
    opts.set<UserProjectOption>(*std::move(e));
  }

+  if (GetEnv("GOOGLE_CLOUD_CPP_OPENTELEMETRY_TRACING")) {


Consider also checking that the result is not empty. That tends to be the general tactic.

I think the value being empty is fine because we don't use the value. Do you think we should be using ParseBoolean(...) instead?

Do you think we should be using ParseBoolean(...) instead?

Possibly. (I do have half a PR to add some parsing to getenv.h.)

But mostly I'm thinking about distinguishing between being set with an empty -v- non-empty value. For example, just a couple of lines up we do that. (Maybe I'm just too stuck in the past where it was difficult to actually remove something from the environment, so an empty value was usually taken to mean the same thing as unset.)

Anyway, I'll leave it to you.

I'm going to treat the presence of the env var as a boolean and ignore its value.

I added a test case for the empty env var to fully enshrine my decision.

devbww · 2023-08-09T16:47:32Z

google/cloud/internal/populate_common_options.cc

@@ -58,8 +59,11 @@ Options PopulateCommonOptions(Options opts, std::string const& endpoint_env_var,
    opts.set<UserProjectOption>(*std::move(e));
  }

+  if (GetEnv("GOOGLE_CLOUD_CPP_OPENTELEMETRY_TRACING")) {


Do you think we should be using ParseBoolean(...) instead?

Possibly. (I do have half a PR to add some parsing to getenv.h.)

But mostly I'm thinking about distinguishing between being set with an empty -v- non-empty value. For example, just a couple of lines up we do that. (Maybe I'm just too stuck in the past where it was difficult to actually remove something from the environment, so an empty value was usually taken to mean the same thing as unset.)

Anyway, I'll leave it to you.

coryan added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 15, 2023

dbolduc removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jun 16, 2023

dbolduc added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 3, 2023

dbolduc force-pushed the otel-tracing-env-var branch from fab82f2 to cbdb770 Compare August 8, 2023 16:53

dbolduc temporarily deployed to internal August 8, 2023 16:53 — with GitHub Actions Inactive

dbolduc removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 8, 2023

dbolduc marked this pull request as ready for review August 8, 2023 17:52

dbolduc requested a review from a team as a code owner August 8, 2023 17:52

coryan reviewed Aug 8, 2023

View reviewed changes

dbolduc temporarily deployed to internal August 9, 2023 05:36 — with GitHub Actions Inactive

coryan approved these changes Aug 9, 2023

View reviewed changes

devbww reviewed Aug 9, 2023

View reviewed changes

devbww approved these changes Aug 9, 2023

View reviewed changes

dbolduc added 4 commits August 9, 2023 12:56

feat(otel): control tracing with environment variable

cbbe761

option always exists

12930d9

address review comments

72dccda

add test case

16316d5

dbolduc force-pushed the otel-tracing-env-var branch from d64db9b to 16316d5 Compare August 9, 2023 16:56

dbolduc temporarily deployed to internal August 9, 2023 16:56 — with GitHub Actions Inactive

capitulate

2d7c0e2

dbolduc temporarily deployed to internal August 9, 2023 18:09 — with GitHub Actions Inactive

dbolduc merged commit d84d5b9 into googleapis:main Aug 10, 2023

dbolduc deleted the otel-tracing-env-var branch August 10, 2023 02:24

dbolduc mentioned this pull request Aug 21, 2023

Consider making OpenTelemetryTracingOption always available #12316

Closed

dbolduc mentioned this pull request Aug 4, 2024

Avoid infinite Cloud Trace export loop #14611

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(otel): control tracing with environment variable #11897

feat(otel): control tracing with environment variable #11897

dbolduc commented Jun 14, 2023 •

edited

Loading

codecov bot commented Jun 15, 2023 •

edited

Loading

coryan commented Jul 21, 2023

coryan Aug 8, 2023

dbolduc Aug 8, 2023 •

edited

Loading

coryan Aug 8, 2023

dbolduc Aug 8, 2023

coryan Aug 8, 2023

dbolduc Aug 8, 2023

devbww Aug 9, 2023

dbolduc Aug 9, 2023

devbww Aug 9, 2023

dbolduc Aug 9, 2023

dbolduc Aug 9, 2023

devbww Aug 9, 2023

feat(otel): control tracing with environment variable #11897

feat(otel): control tracing with environment variable #11897

Conversation

dbolduc commented Jun 14, 2023 • edited Loading

codecov bot commented Jun 15, 2023 • edited Loading

Codecov Report

coryan commented Jul 21, 2023

Choose a reason for hiding this comment

dbolduc Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbolduc commented Jun 14, 2023 •

edited

Loading

codecov bot commented Jun 15, 2023 •

edited

Loading

dbolduc Aug 8, 2023 •

edited

Loading