-
Notifications
You must be signed in to change notification settings - Fork 433
Description
I have seen the Cloud Trace exporter get into an export loop when GOOGLE_CLOUD_CPP_OPENTELEMETRY_TRACING is set.
Past design decision: #11897 (comment)
The exporter is implemented by a trace client. The trace client is fully generated. Also, env vars take precedence over options, so we do not have a good way to override this behavior. Note that the tracing stub / connections are included when the connection is created. So we cannot override the behavior with a client option or a call option.
Proposed Fix
I think we should just explicitly set OpenTelemetryTracingOption == false in the options defaulter iff it is the cloud trace client.
We would verify this with a simple unit test which I would put in google/cloud/opentelemetry. (I don't want to put it in google/cloud/trace because it may complicate the build scripts. google/cloud/opentelemetry already has custom builds).
Alternatives Considered
A. Add some internal::DisableTracingOption, which only gets set in the Cloud Trace exporter.
internal options are a bit of a smell.
B. Decide that the OpenTelemetryTracingOption takes precedence over the environment variable.
Veto. We already released these options/env vars and can't override existing behavior.
C. Have the exporter drop spans named "trace_v2::TraceClient::BatchWriteSpans" or "google.devtools.cloudtrace.TraceService/BatchWriteSpans"
Pro:
- No changes to common
Cons: - Magic strings. If we update the naming conventions of the span, we need to remember to also update the exporter. If we do remember to use this, we are safe. (It is not like there will be a version mismatch between the exporter and the underlying trace client. We have one version for all of
google-cloud-cpp.) - There is no way to trace the exports, even if you wanted to. They happen in band. It could lead to blocks in the application that we would like to explain?
- If gRPC is instrumented, and applications enable tracing, now we have the versioning problem.
- Hm... This seems like an unavoidable problem.
D. Add some sort of short-circuit in the exporter?
E. Override the environment before creating trace client in exporter
No, we should avoid setenv in library code.
F. Tracing layers could check for OpenTelemetryTracingOption == false before tracing.
This one isn't a bad option.