open-telemetry / opentelemetry-go-contrib

Collection of extensions for OpenTelemetry-Go.
https://opentelemetry.io/
Apache License 2.0
1.21k stars 564 forks source link

Allow Disabling Trace Exporting by Default Without Environment Variable Tampering #5194

Open gotgelf opened 8 months ago

gotgelf commented 8 months ago

Problem Statement

In the current implementation, trace exporting is enabled by default (OTLP). This can cause issues because it repeatedly tries to connect to a tracing service that hasn't been set up, leading to a lot of error messages. While some people who use OpenTelemetry might expect it to send data to a local tracing service by default, this automatic behavior may not be suitable for all types of software, especially those used by others in different settings. For example, well-known software like Nginx or Envoy doesn't turn on tracing automatically.

Proposed Solution

Introduce a programmatically accessible mechanism to disable trace exporting by default in the OpenTelemetry Go SDK's autoexport package. This would allow applications to disable tracing without relying on environment variable manipulation, providing a clearer and more direct way to control telemetry features.

Alternatives

Additional Context

This feature request is driven by the aim to enhance the usability and security of the Distribution project's OpenTelemetry integration, as discussed in Issue #4270.

I am keen to collaborate closely on this matter and would be happy to open a related Pull Request once we reach an agreement on the best way forward.

scorpionknifes commented 8 months ago

Doing some investigating:

In otel spec, otlp is the default for automatically configured exporters: https://github.com/open-telemetry/opentelemetry-specification/blob/v1.30.0/specification/configuration/sdk-environment-variables.md#exporter-selection It looks like the java otel sdk also defaults to export otlp as per spec: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/README.md#exporters

I think this suggestion would need to be requested to opentelemetry-specification instead.

My recommendation would be to use stdouttrace.New(stdouttrace.WithWriter(io.Discard)) which is the equalient to a noopSpanExporter. (or not passing autoexp at all here)

corhere commented 8 months ago

@scorpionknifes we do not want to change the SDK defaults for everyone. We just need a way to change the defaults for our application where the OpenTelemetry-mandated SDK defaults would be inappropriate. And what we are requesting is functionality that the OpenTelemetry Specification (arguably) mandates MUST be possible.

The environment-based configuration MUST have a direct code configuration equivalent.

The autoexport package already has an affordance for the application to override the default span exporter using the WithFallbackSpanExporter option. Nobody has to request changes to the OpenTelemetry specification to change their application's default span exporter to e.g. Zipkin, so why should "none" be any different? Specifically, all we are asking for is a way to write (the equivalent of)

autoexport.WithFallbackSpanExporter(func(ctx context.Context) (trace.SpanExporter, error) {
        return autoexport.noopSpanExporter{}, nil
})

in application code.

My recommendation would be to use stdouttrace.New(stdouttrace.WithWriter(io.Discard)) which is the equalient to a noopSpanExporter.

No, that is not equivalent because autoexport.IsNoneSpanExporter(thatDiscardExporter) == false. That function only returns true for values of concrete type autoexport.noopSpanExporter.

(or not passing autoexp at all here)

What exactly are you recommending; that we not use the autoexport package at all to configure tracing in our application? Our users need to be able to configure the application to export traces using the standard environment variables.

scorpionknifes commented 8 months ago

Hey @corhere, thanks for the feedback and more context. Thanks for highlighting that autoexport.WithFallbackSpanExporter exist. I believe you can use a copy of noopSpanExporter just as you suggested in https://github.com/distribution/distribution/issues/4270#issuecomment-1944457010 as IsNoneSpanExporter is not used to determine if a span is exported and is not used internally. I believe the usage of IsNoneSpanExporter is to check which exporter is being returned by autoexport.

What exactly are you recommending; that we not use the autoexport package at all to configure tracing in our application? Our users need to be able to configure the application to export traces using the standard environment variables.

Originally I was recommending to default to not use autoexport if the env is not set, I think the autoexport.WithFallbackSpanExporter approach is much better.

I believe autoexport.noopSpanExporter is not exported to prevent users from misusing it as a generic noopSpanExporter, not sure if there are any special intents here. I would assume if there is a noopSpanExporter available it won't be in the autoexport pkg. I think the closest thing is tracetest.NoopExporter

corhere commented 8 months ago

While IsNoneSpanExporter is not used by opentelemetry-go-contrib, it is useful for applications. If a copy of a no-op span exporter was to be used as the fallback, autoexport.NewSpanExporter would return a different concrete exporter for the fallback than the one returned when the environment variable OTEL_TRACES_EXPORTER=none is set. Applications which need to know whether the span exporter is a no-op exporter would have to consistently check for both types of no-op exporter everywhere. It would be too easy to accidentally check for one but not the other, leading to inconsistent behaviour between the explicit-none and fallback-none cases: an application bug.

I believe autoexport.noopSpanExporter is not exported to prevent users from misusing it as a generic noopSpanExporter

The noopSpanExporter doesn't necessarily have to be exported. One option would be to export a spanExporterFactory function compatible with WithFallbackSpanExporter:

package autoexport

// NoneExporterFactory can be used with the [WithFallbackSpanExporter] option
// to use "none" as the fallback span exporter.
func NoneExporterFactory(ctx context.Context) (trace.SpanExporter, error) {
    return noopSpanExporter{}, nil
}

If the above is still too easy to misuse, the factory could be hidden away entirely inside a special-case span option:

package autoexport

func WithFallbackSpanExporterNone() SpanOption {
    return withFallbackFactory[trace.SpanExporter](func(ctx context.Context) (trace.SpanExporter, error) {
        return autoexport.noopSpanExporter{}, nil
    })
}

Or a new option could be introduced which requires no special casing:

package autoexport

// WithRegisteredFallbackSpanExporter sets the fallback exporter to use
// when no exporter is configured through the OTEL_TRACES_EXPORTER
// environment variable. This function will panic if name is not
// "none", "otlp", "console" or an exporter registered with
// [RegisterSpanExporter].
func WithRegisteredFallbackSpanExporter(name string) SpanOption