Improve autoconfiguration capabilities for adding, delaying or dropping spans

[JonasKunz]

The goal of this PR is to start a discussion around enhancing the ability to modify spans before export.

The existing mechanism to do this are the following:

• SpanProcessor for modifying spans on start
• wrapping SpanExporters for modifying spans before they are exported (= after they have ended)

These options only allow to modify the contents of spans, but they do not allow to efficiently perform the following changes:

• Filtering spans, so that they are not exported
  • See for example this slack discussion
• Delaying spans
  • E.g. in order to add profiling statistics to spans as attributes. Those statistics are likely not immediately available on span end.
• Adding artificial spans
  • Example use-case: Generating spans from profiling data. Could be done via a normal Tracer, but this would technically break the SpanProcessor contracts because onStart / onEnd will not be invoked synchronously on the thread to which the span "belongs".

In theory these actions can already be performed by wrapping SpanExporters. This is however not a good idea, because the addition, delay or dropping would happen after the batching performed by BatchSpanProcessor.
As a result, it essentially breaks batching, making it not really suitable for production.

A batching-friendly alternative would be to wrap the BatchSpanProcessor instead of the SpanExporters:

BatchSpanProcessor exportProcessor = BatchSpanProcessor.builder(myExporter).build();

SdkTracerProvider.builder()
  ...
  .addSpanProcessor(new MyWrapper(exportProcessor))

This approach already works nicely for manually configured SDK instances.
Unfortunately there is no corresponding hook available in the SDK autoconfiguration mechanism.
The code changes in this PR adds a corresponding hook, which could be made experimental for a start.

Note that this PR is intended as just a starting point for a discussion rather than a ready-to-merge suggestion. I'm more than happy to discuss totally different alternatives.

If we decide to go the route suggested in this PR, I'll add test cases which I have omitted for a start.

PR has been updated and enhanced with a test case.

[jackshirazi]

@JonasKunz please implement more completely

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (9ac678e) 91.18% compared to head (49b4f35) 91.16%.
Report is 30 commits behind head on main.

❗ Current head 49b4f35 differs from pull request most recent head a6f79f3. Consider uploading reports for the commit a6f79f3 to get more accurate results

Files	Patch %	Lines
...autoconfigure/spi/AutoConfigurationCustomizer.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #5986      +/-   ##
============================================
- Coverage     91.18%   91.16%   -0.02%     
- Complexity     5613     5616       +3     
============================================
  Files           616      616              
  Lines         16566    16578      +12     
  Branches       1642     1643       +1     
============================================
+ Hits          15105    15114       +9     
- Misses         1013     1014       +1     
- Partials        448      450       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JonasKunz]

@open-telemetry/java-maintainers I've completed the implementation, added a test-case and fixed the failing checks as requested in the last SIG-Meeting. Let me know if you need anything else from my side to move ahead with this PR.

[jack-berg]

Suggested change

	default AutoConfigurationCustomizer addBatchSpanProcessorCustomizer(
	default AutoConfigurationCustomizer addSpanProcessorCustomizer(

[trask]

will this customizer be applied to all span processors, or only to the batch span processor?

[jack-berg]

As written in this PR currently it applies to the result of SpanProcessor.composite, which may be a batch processor or a composite processor depending on whether one or multiple exporters are configured.

I don't see why we would want to limit it to batch processor. Right now, there is no way for other processors to be automatically configured besides simple and batch processor, but with file configuration that will change. If we end up providing the ability to customize the components (notably, I don't think we should allow customizing components), then there will be other potential processors besides simple and batch.

Limiting to batch processor seems to be an overly restrictive API design, even if in practice the batch processor is almost always the processor.

[JonasKunz]

Why did I go for customizing the result of SpanProcessor.composite instead of the individual span processors?
Consider the use-case from my initial comment:

Delaying spans
E.g. in order to add profiling statistics to spans as attributes. Those statistics are likely not immediately available on span end.

The idea here was, that even if multiple span-exporting SpanProcessors are registered, the "delaying" done by the customizer and resulting buffering only needs to be done once per span.
If you instead wrap each individual SpanProcessor, you'll need to do the buffering for each one: E.g. if you have both a SimpleSpanProcessor and a BatchSpanProcessor registered, you would end up with double the memory consumed for buffering and doing the correlation with the data you are waiting for two times.

In production environments I would however expect that you only have a single BatchSpanProcessor and no SimpleSpanProcessor, so the proposal to wrap individual SpanProcessors is fine for me. I'll then just remove the following guarantee from the javadoc:

The provided function will be invoked at most once per customized SDK instance.

Right now, there is no way for other processors to be automatically configured besides simple and batch processor, but with file configuration that will change.

I'm curious, do you have any link to an Issue/PR/something to read up on that feature?

I originally named the method addSpanExportingProcessorCustomizer to explicitly state that the customizer would only be invoked for SpanProcessors responsible for invoking SpanExporters. The reason is that I did not want to wrap other SpanProcessors, e.g. ones responsible for lifting baggage into span-attributes.

I guess with your proposal this still can be done by doing an instanceof BatchSpanProcessor check in the customizer, though this feels a little hacky.

[jack-berg]

Right now, there is no way for other processors to be automatically configured besides simple and batch processor, but with file configuration that will change.

I'm curious, do you have any link to an Issue/PR/something to read up on that feature?

Here are some resources:

• The original OTEP
• The relevant portion of the specification
• The opentelemetry-configuration repository defining the file schema
• The Configuration SIG meets every other monday at 8am PT. Details here.

I guess with your proposal this still can be done by doing an instanceof BatchSpanProcessor check in the customizer, though this feels a little hacky.

This is the expectation right now with all the customizers (i.e. addSamplerCustomizer, add{Signal}ExporterCustomizer, addPropagatorCustomizer), so I think its the right decision.

Another feature of the customizers is they give you the ability to fully replace the component in addition to the ability to wrap it with a delegating class. I'm imagine you could use this with processor customizer for an additional use case: Suppose you don't want to use the batch processor at all, but you know that autoconfigured exporters always get paired with a batch processor. If we add an additional getExporter() method to BatchSpanProcessor, then you could use a span processor customizer to replace the batch processor with another, while retaining the autoconfigured exporter. Something like:

    autoConfiguration.addSpanProcessorCustomizer(
        (spanProcessor, configProperties) -> {
          if (spanProcessor instanceof BatchSpanProcessor) {
            SpanExporter exporter = ((BatchSpanProcessor) spanProcessor).getExporter();
            return new MyCustomProcessor(exporter);
          }
          return spanProcessor;
        });

Pretty slick, IMO.

[JonasKunz]

That's neat!

I also figured it is still relatively easily possible to have a single wrapper wrap all BatchingSpanProcessors and SimpleSpanProcessors for the efficiency I mentioned above:

The idea here was, that even if multiple span-exporting SpanProcessors are registered, the "delaying" done by the customizer and resulting buffering only needs to be done once per span.
If you instead wrap each individual SpanProcessor, you'll need to do the buffering for each one: E.g. if you have both a SimpleSpanProcessor and a BatchSpanProcessor registered, you would end up with double the memory consumed for buffering and doing the correlation with the data you are waiting for two times.

This can be done by providing a stateful BiFunction to the customizer:

• On the first invocation with a BatchSpanProcessor or SimpleSpanProcessor, the processor is wrapped and returned
• The created wrapper is memoized by the BiFunction
• On subsequent invocations with a BatchSpanProcessor or SimpleSpanProcessor, the processor is added to existing wrapper and a NoOp-Processor is returned