Explicitly allow fail-fast for invalid enum values

[pellared]

Why

For the auto-instrumentation scenario, we find that crashing the application in case of an invalid configuration is better. The reason is that it gives the users immediate feedback and forces them to fix the issue. Otherwise, it is likely that will find it when it is too late. For example, they want to analyze the telemetry when something strange is going on with the application; it would occur that someone typed jeager instead of jaeger for OTEL_TRACES_EXPORTER, and because of that, they will miss spans in their backend.

This is our preference in .NET Auto-Instrumentation. Reference:

• https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/blob/main/docs/design.md#error-handling
• Fail fast vs silently continue for wrong env var values opentelemetry-dotnet-instrumentation#952

What

Added or cause the application to fail on initialization possibility when the user provides an enum value the SDK does not recognize.

AFAIK this is how the .NET SDK is working. See: https://github.com/open-telemetry/opentelemetry-dotnet/blob/a37198c6d0f1814f8f5a995413649436657a2e2b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs#L78

Changes

Make the enum value description compatible with the basic error handling principle:

opentelemetry-specification/specification/error-handling.md

Line 18 in 1fd2db2

2. The API or SDK MAY _fail fast_ and cause the application to fail on initialization, e.g. because of a bad user config or environment, but MUST NOT cause the application to fail later at run time, e.g. due to dynamic config settings received from the Collector.

I think it is a backward compatible change as the old behavior is still acceptable per spec.

FYI @open-telemetry/dotnet-instrumentation-approvers

[pellared]

PTAL @open-telemetry/dotnet-maintainers

[pellared]

The markdown-link-check build failures are not related to this PR.

[nrcventura]

I think this ability to fail fast is particularly important for the .NET auto-instrumentation project, because it is the best way for us to provide something to our users to let them know that the configuration is bad and needs to be fixed. Alternative options would likely lead to potentially unexpected behavior, such as no telemetry data being available. With the .net auto-instrumentation project developers may not even have access to the warnings that could be generated from an invalid enum, depending on how their how responsibilities are separated (separate observability team vs direct control of observability).

[reyang]

I don't understand how would this proposal work - imagine the environment variable is used by some exporter, and that exporter is enabled at a later stage (e.g. the application is using some dynamic setting to allow exporters to be loaded and initialized), the wrong environment variable will cause the application to crash.

[pellared]

@reyang PTAL 8b28ee8

I made it explict that it cannot fail later at runtime; stolen from:

opentelemetry-specification/specification/error-handling.md

Line 18 in 1fd2db2

2. The API or SDK MAY _fail fast_ and cause the application to fail on initialization, e.g. because of a bad user config or environment, but MUST NOT cause the application to fail later at run time, e.g. due to dynamic config settings received from the Collector.

[pellared]

PTAL @open-telemetry/technical-committee @open-telemetry/specs-approvers

[tigrannajaryan]

I think this change results in more desirable behavior however it is not clear to me if this is an allowed breaking change or no. The document is marked as "Mixed" and there is no status in the "Enum value" section. For "Mixed" document do we implicitly assume unmarked sections to be "Stable" or "Experimental"?
I will approve if we believe this is an allowed change.

[pellared]

it is not clear to me if this is an allowed breaking change or no

I think it is a backward compatible change as the old functionality is still acceptable per spec. Personally, I see it more as clarification (or even fix) as this meets the basic error handling guidelines.

The document is marked as "Mixed" and there is no status in the "Enum value" section. For "Mixed" document do we implicitly assume unmarked sections to be "Stable" or "Experimental"?

This is a subject for a separate PR/discussion. We can mark it as stable in a following PR or create issue(s) to define what is missing to mark it as stable.

The whole "Special configuration types" section is marked as "Stable". Then I would say that "Enum value" section is "Stable" as well.

[pellared]

I updated the description to describe some background.

[tigrannajaryan]

The whole "Special configuration types" section is marked as "Stable". Then I would say that "Enum value" section is "Stable" as well.

I think this means this PR makes a breaking change to a "Stable" section of the spec. An application that previously would not fail can now fail after updating to a newer SDK that implements this change. That is not allowed.

Now, there is one argument that can be made to still allow this change: .NET SDK already works as this PR describes - it fails fast. Question: what do other language SDKs do currently? Do they fail fast or ignore the invalid values?

If all/most other SDKs also fail fast then I think we can say that the spec has a bug and what is written does not match the reality.

[pellared]

An application that previously would not fail can now fail after updating to a newer SDK that implements this change.

That would be indeed a breaking change but in the SDK 😉 But I do not have a strong opinion here.

[carlosalberto]

As discussed in yesterday's Spec call, one alternative is to fail fast when using a debug mode flag (maybe set through an environment variable). See #2817

Would this help at all for your considered case @pellared ?

[pellared]

As discussed in yesterday's Spec call, one alternative is to fail fast when using a debug mode flag (maybe set through an environment variable). See #2817

Would this help at all for your considered case @pellared ?

@carlosalberto
Kind of yes. Thanks a lot 😉 I also answered in the new issue.

I have also a different question. Is "auto-instrumentation" seen as "SDK"? Maybe it is reasonable that some behavior in auto-instrumentation is different? E.g. fail-fast becasue the initializtion is done before the application is even started. I cannot find anything in the spec that would clarify it.

@reyang @cijothomas I created open-telemetry/opentelemetry-dotnet#3690

[yurishkuro]

I am not sure why this behavior is only under Enum values, shouldn't there be a common error behavior for all env values? What if integer var is set to blah?

[pellared]

I was thinking about it as well, but I want to avoid scope creep in this PR. Even right now it is not clear how to proceed

[yurishkuro]

I don't think this change is necessary - #2817 (comment)

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.