StackOverflowError when using propagation type custom

[qavid]

Describe the bug
When using propagation type custom by setting property:
spring.sleuth.propagation.type=custom
StackOverflowError is thrown when Tracing bean is initialized.

As Tracing bean is lazily initialized, you can for example provide tracing customizer:

@Bean
public NettyServerCustomizer tracingCustomizer(HttpTracing httpTracing) {
    return server -> ReactorNettyHttpTracing.create(httpTracing).decorateHttpServer(server);
}

and exception will be thrown at startup.

Problem is reproducible with spring-cloud-sleuth version 3.0.3. Previous version 3.0.2 didn't have this problem.
Problem relates to newly introduced LazyPropagationFactory (in CompositePropagationFactory).

Sample
spring-cloud-samples/sleuth-issues#73

[marcingrzejszczak]

I'm looking into that and I know what's going on. So we're setting custom, I register a Propagation.Factory sleuthPropagationWithB3Baggage that returns a BaggageFactoryWrapper that calls the supplier that returns the Propagation.Factory later on and it returns... BaggageFactoryWrapper 😬 I'm starting to wonder what the point of CUSTOM is... If someone sets to CUSTOM, then what should really happen? We even write this in the doc that it's enough to write sth like this:

class CustomPropagator extends Propagation.Factory implements Propagation<String> {

	@Override
	public List<String> keys() {
		return Arrays.asList("myCustomTraceId", "myCustomSpanId");
	}

	@Override
	public <R> TraceContext.Injector<R> injector(Setter<R, String> setter) {
		return (traceContext, request) -> {
			setter.put(request, "myCustomTraceId", traceContext.traceIdString());
			setter.put(request, "myCustomSpanId", traceContext.spanIdString());
		};
	}

	@Override
	public <R> TraceContext.Extractor<R> extractor(Getter<R, String> getter) {
		return request -> TraceContextOrSamplingFlags.create(TraceContext.newBuilder()
				.traceId(HexCodec.lowerHexToUnsignedLong(getter.get(request, "myCustomTraceId")))
				.spanId(HexCodec.lowerHexToUnsignedLong(getter.get(request, "myCustomSpanId"))).build());
	}

	@Override
	public <K> Propagation<K> create(KeyFactory<K> keyFactory) {
		return StringPropagationAdapter.create(this, keyFactory);
	}

}

Then irrelevant of the propagation type property this factory will be registered. I think we should remove the CUSTOM property cause it makes no sense. WDYT? cc @jonatan-ivanov

[jonatan-ivanov]

@marcingrzejszczak I think removing CUSTOM makes sense if the logic will be governed by having/not having a bean.
Though the interesting/weird part: having the bean -> custom propagation and having a property that said propagation=W3C.

[jonatan-ivanov]

@qavid Could you please try this with the latest snapshot (it should be published in a few minutes)?

[marcingrzejszczak]

So how i wrote it is that you can have custom however you need to provide a proper beans registered. If you don't we throw an exception

[jonatan-ivanov]

👍🏼

[qavid]

@marcingrzejszczak @jonatan-ivanov thanks for explanation. We use custom PropagationFactorySupplier now. It's not exactly what we wanted, but it solves our problem.

Just to explain our original intent...

We used CUSTOM propagation because we wanted to:
extract baggage fields from incoming request propagate baggage fields in reactor context, correlate baggage fields in logs and we didn't wanted inject baggage fields (or any other tracing fields) into outgoing requests (NoOpPropagation).

In other words we wanted BaggagePropagation.Factory (defined as sleuthPropagationWithB3Baggage in BraveBaggageConfiguration) wich delegates to CompositePropagationFactory which delegates based on spring.sleuth.propagation.type to proper Propagation.Factory (in previous version, CUSTOM defaulted to NoOpPropagation).

We wanted use minimal custom configuration (depending only on autoconfiguration) and be able to switch to any supported propagation by configuration change. So we wanted something like NONE propagation.

Do you think you could make NoOpPropagation (Propagation.Factory) public (or at least its reference) so we don't have to copy same implementation?

[marcingrzejszczak]

You have an extremely custom case. Noop means no propagation at all.