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

autoexport: Support none as exporter #4013

Closed
pellared opened this issue Jun 20, 2023 · 4 comments · Fixed by #4130
Closed

autoexport: Support none as exporter #4013

pellared opened this issue Jun 20, 2023 · 4 comments · Fixed by #4130
Assignees
Labels
area: exporter Related to an exporter package enhancement New feature or request

Comments

@pellared
Copy link
Member

Is there a no-op exporter we can use with autoexport?

Originally posted by @james-callahan in #2753 (comment)

@pellared
Copy link
Member Author

pellared commented Jun 20, 2023

From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#enum-value:

If a null object (empty, no-op) value is acceptable, then the enum value representing it MUST be "none".

From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk-environment-variables.md#exporter-selection:

"none": No automatically configured exporter for [...]

@MikeGoldsmith Do you want to work on it?

@pellared pellared added area: exporter Related to an exporter package enhancement New feature or request labels Jun 20, 2023
@pellared pellared self-assigned this Jul 25, 2023
@pellared
Copy link
Member Author

pellared commented Jul 28, 2023

How do you prefer func NewSpanExporter(ctx context.Context, opts ...Option) (trace.SpanExporter, error) { to behave when user selects none?

I see following possibilities

Option A - return noopExporter{}, nil

Pros:

  1. Ease of use by the user - no special handling
  2. This would align with the behavior of autoprop.NewTextMapPropagator

Cons:

  1. Performance downgrade. If none is selected then there may be no need to even setup a metrics provider. How the user would know that a "None" (noop) exporter was returned. MITIGATION: We can add a IsNone function so that the user could handle the special case when none is selected.

Option B - return nil, nil

Pros:

  1. Easy to implement

Cons:

  1. It is bug prone for the user. It may be easy for the user to configure a nil exporter as return nil, nil is not very popular (but it happens). Users may not read the documentation and not handle this case.
  2. It requires handling a if exp == nil case

Option C - return nil, ErrNone

Pros:

  1. Looks idiomatic
  2. If user does not handle the exporter err == ErrNone he will handle it like an error. It is less bug prone than option A.

Cons:

  1. This is not really an error.
  2. The error handling code may look worse than for option A. In most cases setting a NOOP exporter should be good enought.

I am leaning towards Option A - return noopExporter{}, nil.

@open-telemetry/go-approvers, WDYT?

@james-callahan
Copy link

  1. Performance downgrade. If none is selected then there may be no need to even setup a metrics provider. How the user would know that a "None" (noop) exporter was returned. MITIGATION: We can add a IsNone function so that the user could handle the special case when none is selected.

I think the selection of exporter should be orthogonal to the metrics provider (and other things).
In particular, I want to be able to turn on and configure propagation, but turn off exporting spans.
If the exporter throws away the span; it's no different to if the user was e.g. using a UDP exporter and experienced packet loss: the exporter saw the span.


I think option A makes the most sense. I wouldn't provide any sort of special IsNone function.

@dashpole
Copy link
Contributor

I prefer A as well.

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
3 participants