Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
gotgelf opened this issue Mar 1, 2024 · 4 comments
Labels
area: exporter Related to an exporter package enhancement New feature or request

Comments

@gotgelf
Copy link

gotgelf commented Mar 1, 2024

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

  • Change the default exporter type: Modify the autoexport/signal.go file to set the default expType to none instead of otlp. This straightforward approach would ensure that, in the absence of explicit configuration, no trace data is exported.

  • Export noopSpanExporter: Making the noopSpanExporter type exportable would allow developers to explicitly set a no-operation span exporter. This method provides flexibility while ensuring that trace exporting can be disabled programmatically.

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
Copy link
Member

scorpionknifes commented Mar 5, 2024

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
Copy link

corhere commented Mar 6, 2024

@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
Copy link
Member

scorpionknifes commented Mar 6, 2024

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 distribution/distribution#4270 (comment) 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
Copy link

corhere commented Mar 6, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: exporter Related to an exporter package enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants