MultiSpanProcessor caches method values of delegates

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);
  }

Start with MultiSpanProcessor and the SpanProcessor interface, then reproduce the behavior with the respectNeeds test shown in the issue. Check how isStartRequired() and isEndRequired() are used during construction and dispatch. Done means the documented or agreed behavior is captured by a passing regression test, with any compatibility implications resolved for a future major version.