SpanProcessor called even when isStartRequired / isEndRequired are false

[jack-berg]

The following test shows a span processor which doesn't require start or end, and despite this, onStart and onEnd are called! Yikes!

@Test
void foo() {
    AtomicLong onStartCount = new AtomicLong();
    AtomicLong onEndCount = new AtomicLong();

    SpanProcessor processor = new SpanProcessor() {
        @Override
        public void onStart(Context parentContext, ReadWriteSpan span) {
            onStartCount.incrementAndGet();
        }

        @Override
        public boolean isStartRequired() {
            return false;
        }

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

        @Override
        public boolean isEndRequired() {
            return false;
        }
    };

    SdkTracerProvider sdkTracerProvider = SdkTracerProvider.builder()
            .addSpanProcessor(processor)
            .build();

    Tracer tracer = sdkTracerProvider.get("tracer");

    for (int i = 0; i < 100; i++) {
        tracer.spanBuilder("span").startSpan().end();
    }

    assertThat(onStartCount.get()).isEqualTo(0);
    assertThat(onEndCount.get()).isEqualTo(0);
}

[jkwatson]

Seems like a minor bug...why would you build a span processor that literally didn't want to ever see spans, either on start or on finish/

[jack-berg]

why would you build a span processor that literally didn't want to ever see spans, either on start or on finish/

This shows we call onEnd even if only isOnStartRequired=true, and we call onStart even if only isOnEnd=true.

[jkwatson]

why would you build a span processor that literally didn't want to ever see spans, either on start or on finish/

This shows we call onEnd even if only isOnStartRequired=true, and we call onStart even if only isOnEnd=true.

The test case posted above has both set to false. Am I missing something?

[jack-berg]

Sorry I mispoke. The example shows isStartRequired=false and isEndRequired=false, and both getting called anyway. But it doesn't actually matter what isStartRequired and isEndRequired are set to. In general, if only on span processor is added, the responses of isStartRequired and isEndRequired are ignored and both onStart and onEnd are always called.

[jkwatson]

🤔 has this always been the case? what a weird bug to last this long.

[jack-berg]

Not sure, I think probably yes. Oddly, we rely on MultiSpanProcessor to decide whether to call onStart / onEnd. Its constructor is the only place in the code base which calls isStartRequired / isEndRequired. So the bug exists because we transform all the registered processors into a single MultiSpanProcessor, except when there's exactly 1 processor because of this code.

It is definitely a weird bug to have made it this far.