Refactor exporter - step 1

[reyang]

Trying to fix several issues in the current exporter design. Related to #896 and #896 (comment).

The overall thinking:

1. We don't want to have two queues - one in the core SDK and another in the exporter. The exporter shouldn't handle queuing at all, that is the responsibility of batch export processor.
2. Processor/Exporter shouldn't shoot Task randomly to the worker pool and have nondeterministic behavior or unbounded memory usage.
3. Simple export processor should be simple, it shouldn't have any in-memory queue.
4. Jaeger/Zipkin/Otlp exporters should use batch export processor by default.
5. ActivityListener is sync and dispose is also sync, it doesn't make much sense to have async interface in the exporter. The actual exporter implementation can always use async pattern as they would want. This would give memory allocation benefit to sync exporters.
6. SimpleActivityProcessor should be renamed to SimpleExportActivityProcessor to avoid the confusion (and the same for batch exporter) - OpenTelemetry Python is doing the right job here.
7. maxExportBatchSize should be enforced - the exporter should never receive a batch with more than maxExportBatchSize items specified in the BatchExportActivityProcessor.

Changes

• Added a new class ActivityExporterSync, which I plan to replace the current ActivityExporter in the next couple PRs (and rename back to ActivityExporter after the refactor is done).
• I've also changed the result code to Success and Failure, as the spec is saying there are only two values.
• Similar like the provider/processor refactor, will send a list of small PRs, step by step, and add test cases after the refactor is done.

For significant contributions please make sure you have completed the following items:

• Design discussion issue #
• Changes in public API reviewed

[reyang]

@cijothomas @CodeBlanch

[reyang]

I think this IEnumerable should be replaced with something that doesn't have heap allocation, not trying to cover it in this PR.

[CodeBlanch]

❤️ this! Just got this merged so we can prevent allocations doing foreachs on the Activity data (tags, links, events, etc.): dotnet/runtime#40544

[reyang]

#1080 for tracking.

[codecov]

Codecov Report

Merging #1078 into master will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1078      +/-   ##
==========================================
- Coverage   77.40%   77.33%   -0.07%     
==========================================
  Files         220      221       +1     
  Lines        6080     6085       +5     
==========================================
  Hits         4706     4706              
- Misses       1374     1379       +5     

Impacted Files	Coverage Δ
src/OpenTelemetry/Trace/ActivityExporterSync.cs	0.00% <0.00%> (ø)

[cijothomas]

should we simply use a bool to indicate success/failure?

[reyang]

I guess we want to keep close to the spec.

[cijothomas]

Now I wonder why spec didn't simply use a boolean? I guess its leftover from old behavior where ExportResult had more than 2 possible values!

[reyang]

I guess it leaves the room so in the future folks can add more value.

[cijothomas]

mm.. Is there any value in Export() returning anything at all? I mean, irrespective of the return value, the Processor simply moves on to next batch. Is it to let Processor do some logging that batch is being dropped? Or to allow future possibilities..

[reyang]

I don't see BIG value, but I do see value - for example - the processor could log an internal error, or the processor can use this indication to report exporting statistics (e.g. metrics telling folks how many activities are exported successfully vs. dropped).

[alanwest]

the processor could log an internal error, or the processor can use this indication to report exporting statistics

These are definitely interesting use cases, but seems to me to be more of a concern for the exporter itself since it has the underlying knowledge of either the error or statistics.

[reyang]

I agree.

[cijothomas]

ok. lets stick with spec and keep enum with 2 values for now :)

[cijothomas]

LGTM. Will wait till end of day to get additional feedback before merging.

[alanwest]

The spec says "Export() must not block indefinitely, there must be a reasonable upper limit after which the call must time out with an error result (Failure).".

I'm curious what is considered reasonable?

[reyang]

The spec says "Export() must not block indefinitely, there must be a reasonable upper limit after which the call must time out with an error result (Failure).".

I'm curious what is considered reasonable?

I've seen 15 seconds, 1 minutes and some value in between :)
I think individual exporters should have their own clarification of what's reasonable for them.

[CodeBlanch]

@reyang Plan looks solid to me.