MultiSpanProcessor caches method values of delegates

[breedx-splk]

The MultiSpanProcessor is designed to wrap a bunch of other SpanProcessors that it delegates to. SpanProcessor is an interface, and it has two methods isStartRequired() and isEndRequired(). These methods presumably tell the user of the SpanProcessor instance whether or not the corresponding onStart() or onEnd() method should be invoked.

As of this writing, there is only one caller of the isStartRequired() and isEndRequired() methods, and that is the MultiSpanProcessor. One could make the argument that simply allowing implementers to build a no-op onStart() or onEnd() would have been sufficient, but the original authors chose to guard this with another method call instead. Baffling.

In any case, the MultiSpanProcessor seems to have been crafted with premature optimization in mind. In the constructor, the delegates are grouped into two possibly overlapping sets -- those that require start and those that require end. This set creation is done in the constructor by invoking isStartRequired() and isEndRequired() at construction time, and never again. The creation of these sets effectively caches the method call return value for the life of the MultiSpanProcessor.

This might have been the original design, but it feels like a bug. There is nothing in the javadoc discouraging implementers from returning different values from is{Start/End}Required(), and forcing users to implement a method whose return value is subsequently ignored seems like bad design.

Because the SpanProcessor (and all of its friends) are "stable", this will be difficult to change. I suggest that we keep this in scope when looking at a future major version bump. There are definitely some interesting design discussions to be had around this.

Here is an example unit test that uses a SpanProcessor that only wants its onStart() and onEnd() to be called once. The MutiSpanProcessor is called 100 times, and sure enough, the delegate is also called 100 times. This could be a harsh surprise to the implementer!

This test fails today:

  @Test
  void respectNeeds() {
    AtomicInteger startCt = new AtomicInteger();
    AtomicInteger endCt = new AtomicInteger();
    SpanProcessor oncePlease = new SpanProcessor() {

      @Override
      public void onStart(Context parentContext, ReadWriteSpan span) {
        startCt.incrementAndGet();
      }

      @Override
      public boolean isStartRequired() {
        return startCt.get() == 0;
      }

      @Override
      public void onEnd(ReadableSpan span) {
        endCt.incrementAndGet();
      }

      @Override
      public boolean isEndRequired() {
        return endCt.get() == 0;
      }
    };
    SpanProcessor multi = MultiSpanProcessor.create(singletonList(oncePlease));
    for(int i=0; i < 100; i++){
      multi.onStart(null, null);
      multi.onEnd(null);
    }
    assertThat(startCt.get()).isEqualTo(1);
    assertThat(endCt.get()).isEqualTo(1);
  }

[jack-berg]

Yeah this is something I've thought about as well. What you essentially asking is should span processors be allowed to dynamically adjust whether the want to be called on start and end. isStartRequired and isEndRequired are java inventions not part of the SpanProcessor spec, so we have some agency on what their semantics should be.

As of this writing, there is only one caller of the isStartRequired() and isEndRequired() methods, and that is the MultiSpanProcessor.

Behind the scenes the tracer provider SDK groups all registered span processors into one MultiSpanProcessor, so the behavior of MultiSpanProcessor is the effective behavior of the SDK.

Note I believe there's actually a way to trick the SDK into the behavior you want: You should be able to register your own implementation of a "multi span processor" with the semantics you want. As you note, there is no code outside of MultiSpanProcessor which calls isStartRequired / isEndRequired - SdkSpan just calls SpanProcessor#onStart and depends on MultiSpanProcessor to sort out which processors should be called. So if you register exactly one span processor, your processor should be called all the time, regardless of whether it requires start / end. Wait.. now that I write that, that's definitely a bug. Filed #5930 to track.

In any case, the MultiSpanProcessor seems to have been crafted with premature optimization in mind.

I actually think it the correct choice. You can always loosen up the API later (as we're discussing), but restricting would be viewed as breaking.