Add support for OTEL_BSP_EXPORT_* environmental variables

[pellared]

Partially addresses #1453.

This feature is important so that we can reuse the SDK in https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation.

Changes

• The BatchExportProcessorOptions defaults can be overridden using
  OTEL_BSP_SCHEDULE_DELAY, OTEL_BSP_EXPORT_TIMEOUT,
  OTEL_BSP_MAX_QUEUE_SIZE, OTEL_BSP_MAX_EXPORT_BATCH_SIZE
  envionmental variables as defined in the
  specification.

[codecov]

Codecov Report

Merging #2219 (ceae91e) into main (232cc1d) will decrease coverage by 0.03%.
The diff coverage is 79.41%.

@@            Coverage Diff             @@
##             main    #2219      +/-   ##
==========================================
- Coverage   79.66%   79.63%   -0.04%     
==========================================
  Files         232      233       +1     
  Lines        7294     7325      +31     
==========================================
+ Hits         5811     5833      +22     
- Misses       1483     1492       +9     

Impacted Files	Coverage Δ
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs	70.65% <57.14%> (-1.12%)	⬇️
...metry/Trace/BatchExportActivityProcessorOptions.cs	83.33% <83.33%> (ø)
...Telemetry.Exporter.Jaeger/JaegerExporterOptions.cs	84.21% <100.00%> (ø)
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs	84.61% <100.00%> (ø)
...Telemetry.Exporter.Zipkin/ZipkinExporterOptions.cs	100.00% <100.00%> (ø)
...ter.ZPages/Implementation/ZPagesActivityTracker.cs	97.14% <0.00%> (-2.86%)	⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs	96.85% <0.00%> (-0.79%)	⬇️

[reyang]

These are specific to batch span processors, we need some logic to make sure it is not affecting other scenario (e.g. metrics).

[pellared]

Do you have any proposal on how to approach this?

I think that the root problem is that the .NET SDK tries to use common (premature?) abstractions to implement multiple OTel specification components. E.g. we have a common exporter base class for both metrics and traces exporter. I am aware it reduces some code duplication but it tightly couples the API and it may be hard to maintain backward compatibility going this way.

Can we use BatchExportProcessorOptions for tracing (to not break SemVer) and create a BatchMetricsExportProcessorOptions for metrics SDK?

Moreover, I think we should consider separating the metrics and traces SDKs.

[CodeBlanch]

What if we did something like...

        internal const string SpanMaxQueueSizeEnvVarKey = "OTEL_BSP_MAX_QUEUE_SIZE";

        internal const string SpanMaxExportBatchSizeEnvVarKey = "OTEL_BSP_MAX_EXPORT_BATCH_SIZE";

        internal const string SpanExporterTimeoutEnvVarKey = "OTEL_BSP_EXPORT_TIMEOUT";

        internal const string SpanScheduledDelayEnvVarKey = "OTEL_BSP_SCHEDULE_DELAY";

        public BatchExportProcessorOptions()
        {
            if (typeof(T) == typeof(Activity))
            {
               LoadSpanEnvVars();
            }
        }

        private void LoadSpanEnvVars()
        {
            int value;

            if (TryLoadEnvVarInt(SpanExporterTimeoutEnvVarKey, out value))
            {
                this.ExporterTimeoutMilliseconds = value;
            }

            if (TryLoadEnvVarInt(SpanMaxExportBatchSizeEnvVarKey, out value))
            {
                this.MaxExportBatchSize = value;
            }

            if (TryLoadEnvVarInt(SpanMaxQueueSizeEnvVarKey, out value))
            {
                this.MaxQueueSize = value;
            }

            if (TryLoadEnvVarInt(SpanScheduledDelayEnvVarKey, out value))
            {
                this.ScheduledDelayMilliseconds = value;
            }
        }

I'm not really a fan of duck typing in generics like this but trying to introduce classes into the hierarchy now will probably be a breaking change.

[pellared]

@CodeBlanch thanks 👍

but trying to introduce classes into the hierarchy now will probably be a breaking change

That is why I suggest changing the metrics SDK as AFAIK it od not stable yet. And leave the tracing SDK as it is.

Your proposal would work and probably it is the most pragmatic idea for this PR. But maybe it is worth taking a step back and make more refactoring in separate PR(s).

[pellared]

@CodeBlanch
I noticed that OtlpMetricsExporter uses OtlpExporterOptions which has a BatchExportProcessorOptions<Activity>.

Personally, I would prefer to redesign the OtlpMetricsExporter. The main repository's readme says that metrics are experimental so probably it is acceptable. Then we could redesign OtlpMetricsExporter to not have BatchExportProcessorOptions as it is not used anyway. I think we could refactor OtlpMetricsExporter as a separate issue+PR.

Regarding this PR I see the following options:

1. We could just improve the BatchExportProcessorOptions docs and say that this option is designed for the tracing SDK.

2. Instead of "duck typing in generics" we could also possibly create a subclass BatchSpanExportProcessorOptions and use it like this:

   public BatchExportProcessorOptions<Activity> BatchExportProcessorOptions { get; set; } = new BatchSpanExportProcessorOptions();

   not sure how much of a breaking change - I guess 99% of the existing code should simply work. Personally, I find such "common property grouping" as a code smell and possible premature abstraction. Will the exporters handle all of the possible options?

3. Use the "duck typing in generics" proposed here

Shouldn't the tracing and metrics packages be distributed separately to avoid confusion? How the user would distinguish between stable and unstable API?

@reyang PTAL as well

[pellared]

After the SIG meeting, I have an impression that this would be the preferred way forward:

1. Create an issue that OtlpMetricsExporter should not reference BatchExportProcessorOptions<Activity>. Done here: OtlpMetricsExporter should not reference BatchExportProcessorOptions<Activity> #2246
2. Create a BatchSpanExportProcessorOptions as a subclass of BatchExportProcessorOptions<Activity>. Addressed here: 092ddea

[pellared]

@open-telemetry/dotnet-approvers PTAL

[dszmigielski]

@open-telemetry/dotnet-approvers could you PTAL, so we can close this one? It's been hanging here for quite a long time now.

[reyang]

LGTM. Sorry for my delayed response, I was focusing on the metrics part recently.

[cijothomas]

might be a good idea to call out that this env variables affect all exporters unless individually overridden in code.

[cijothomas]

@pellared Merging this. Please take a note of couple of suggestion, and consider follow ups to address them.

[pellared]

@cijothomas @CodeBlanch I have the follow-ups very high on my TODO list. I am just overwhelmed with other stuff ATM.