GoogleCloudPlatform / opentelemetry-operations-go

Apache License 2.0
131 stars 100 forks source link

exporter/trace: Should trace context propagation be disabled in the Cloud Trace client? #864

Closed quartzmo closed 3 weeks ago

quartzmo commented 3 months ago

The OpenTelemetry Google Cloud Trace Exporter uses the Cloud Trace client (cloud.google.com/go/trace) to send user tracing to the Cloud Trace service. However, as a generated client in the google-cloud-go project, it itself has trace context propagation enabled by default. This appears to result in unwanted traces. See:

https://github.com/googleapis/google-api-go-client/issues/2639

@Pascal-Delange wrote:

I would say it's safe to assume that the traces from the trace client sending traces itself are going to be noise >99.99% of the time (which will be more or less visible hence more or less annoying depending on the type of app being traced). (it may still show up if you're profiling the application, but that is a different story) A default behavior of not sending traces would seem reasonable to me.

Should OpenTelemetry context propagation be disabled in the trace client by passing option.WithTelemetryDisabled within OpenTelemetry Google Cloud Trace Exporter? This solution was used by @Pascal-Delange in 2639 and appeared to resolve the unwanted traces. Should this be done for all users?

See also: googleapis/google-cloud-go/issues/10409

dashpole commented 3 months ago

Thats a good question. We probably do want metrics to be enabled by default, and we do want context propagation to be enabled by default, so the only component we would consider disabling by default would be tracing.

Normally, tracing for the tracing client isn't an issue if users configure sampling, since only a small percentage of trace export requests are sampled. It causes issues if users use the (default) 100% sampling rate, as it creates an infinite loop.

The only other concern I would have with setting WithTelemetryDisabled() would be that it doesn't give users the ability to re-enable telemetry if we set it.

codyoss commented 2 months ago

I think this should be off by default for the tracing client. Maybe expose a flag or envvar to enable it. If I was a user of the library that is at least the behaviour I think I would expect. Also I think the infinite loop issue is big enough to warrant it alone.

dashpole commented 1 month ago

@codyoss should we implement this in our otel exporters, or should it be added as an option by default in the trace client itself?

As an aside, I am having trouble reproducing the issue in https://github.com/googleapis/google-api-go-client/issues/2639. For example, the example in https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/tree/main/example/trace/http doesn't appear to produce any spans for the trace client.

codyoss commented 1 month ago

@dashpole I think this for sure should be implemented in the in the exporter, but maybe additionally here as well.

dashpole commented 3 weeks ago

I'm going to close this here then, and suggest we implement it in the exporter library itself.