Auto-configured ExemplarSampler bean only backs off when a DefaultExemplarSampler is defined

[johnnywiller]

Having Spring Actuator autoconfiguring a DefaultExemplarSampler makes it harder for the user to define its own bean of type ExemplarSampler, since @ConditionalOnMissingBean will still create a DefaultExemplarSampler causing two beans of type ExemplarSampler to be created.

This PR simply points to the interface instead.

Another option would be to explicitly add @ConditionalOnMissingBean(ExemplarSampler.class) but seems less correct.

[pivotal-cla]

@johnnywiller Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-cla]

@johnnywiller Thank you for signing the Contributor License Agreement!

[wilkinsona]

Rather than changing the return type, which deprives the bean factory of type information, it would be better to change @ConditionalOnMissingBean to @ConditionalOnMissingBean(ExamplarSampler.class). This will cause exemplarSampler to back off if there's an ExamplarEampler bean defined, while also still telling the bean factory that exemplarSampler is a DefaultExemplarSampler.

[johnnywiller]

Thanks for the review, changed.
I'm not particularly familiar with best practices around bean autoconfiguration and conditional beans idiom, but I though defining interfaces instead of implementation would allow for more overall flexibility.

[philwebb]

When Spring Framework creates bean definitions from @Configuration classes it only has the method signature to go on. It won't actually invoke the method and get the real bean instance until much later.

For that reason, it's always best to use the most specific type in the method signature. That way Spring has as much information as possible.

[wilkinsona]

We should add a test for the backing-off behavior as part of merging this.

[jonatan-ivanov]

Out of curiosity (I think this is a good change that we should merge in): what is your use case for having a custom exemplar sampler?

[johnnywiller]

Out of curiosity (I think this is a good change that we should merge in): what is your use case for having a custom exemplar sampler?

We are decorating the bean DefaultExemplarSampler to apply custom tagging to the spans. The custom tags will be something like "exemplar": "sampled" and serve to inform Otel Collector that it should not drop those spans.
We send metrics directly to Prometheus and Spans/Traces to Otel Collector, so if a metric contains exemplar, the referred span should not be dropped in otel collector.

The decorator looks like

@Bean
@ConditionalOnBean({SpanContextSupplier.class, Tracer.class})
@ConditionalOnMissingBean
DefaultExemplarSampler exemplarMarkingExemplarSampler(SpanContextSupplier spanContextSupplier,
                                                          Tracer tracer) {
        var defaultExemplarSampler = new DefaultExemplarSampler(spanContextSupplier);
        /*
         we are returning a DefaultExemplarSampler instead of ExemplarSampler
         interface because actuator autoconfiguration is doing ConditionalOnBean directly on the
         DefaultExemplarSampler. 
         The ExemplarMarkingExemplarSampler decorator should be changed
         to implement ExemplarSampler once Spring is updated
        */
        return new ExemplarMarkingExemplarSampler(spanContextSupplier, defaultExemplarSampler, tracer);
    }

[jonatan-ivanov]

I'm thinking, if prometheus sampler/supplier and/or Boot should provide this functionality.

I have another quick question: I did not check this so I'm not sure what can go wrong with it but wouldn't this be possible in the SpanContextSupplier where you have access to the Span anyways? So instead of having a ExemplarMarkingExemplarSampler, you would use the DefaultSampler but instead of the SpanContextSupplier that Boot creates (and that one backs off), you can implement a ExemplarMarkingSpanContextSupplier (doing the same just one level deeper, I guess more conveniently)?

[johnnywiller]

I'm thinking, if prometheus sampler/supplier and/or Boot should provide this functionality.

You mean, providing a extension point to add custom tags to the spans, or directly tag a span as exemplar?
The latter seems a bit complicated as I think there is no standard tag for exemplars yet... And for the former, would be cool but not sure how to design such a thing.

wouldn't this be possible in the SpanContextSupplier where you have access to the Span anyways?

We just want to tag spans when they are being used as exemplars on metrics, not for all spans.
I may be wrong but I believe SpanContextSupplier is a more generic bean that can be called whenever there is a need for getting a span, which may or may not be a exemplar situation.
As seem in DefaultExemplarSampler#doSample, spans are only exemplified when they meet the condition:

if ((previous == null || previous.getTimestampMs() == null
        || timestampMs - previous.getTimestampMs() > minRetentionIntervalMs)
        && spanContextSupplier.isSampled()) {

So to me, if feels more solid to add the tagging behaviour close to ExemplarSampler. Here's how we are doing it (partially omitted for brevity):

class ExemplarMarkingExemplarSampler extends DefaultExemplarSampler { // ideally should implement from ExemplarSampler, that's what started this PR
    private final ExemplarSampler delegate;
    private final Tracer tracer;

    @Override
    public Exemplar sample(double increment, Exemplar previous) {
        Exemplar exemplar = delegate.sample(increment, previous);
        if (exemplar != null) {
            markSpanAsExemplar();
        }
        return exemplar;
    }

    private void markSpanAsExemplar() {
        var span = tracer.currentSpan();
        if (span != null) {
            span.tag("exemplar", "sampled");
        }
    }
}

I forgot to mention in the use case description, the otel collector has a TAIL SAMPLING rule to avoid dropping spans with the "exemplar" tag, so the whole purpose is for tail sampling.

[jonatan-ivanov]

You mean, providing a extension point to add custom tags to the spans, or directly tag a span as exemplar?
The latter seems a bit complicated as I think there is no standard tag for exemplars yet... And for the former, would be cool but not sure how to design such a thing.

We can let you inject a Supplier<Span> span customizer so that you can modify your span when it is sampled.

We just want to tag spans when they are being used as exemplars on metrics, not for all spans.
I may be wrong but I believe SpanContextSupplier is a more generic bean that can be called whenever there is a need for getting a span, which may or may not be a exemplar situation.

SpanContextSupplier should be only used when an exemplar is sampled, it is also in the "exemplars" package: io.prometheus.client.exemplars.tracer.common, why do you think it is used elsewhere and what is it used for if so (I can tell that in Micrometer and Boot it is only used to sample exemplars)?

As seem in DefaultExemplarSampler#doSample, spans are only exemplified when they meet the condition:...

I'm not sure what is the relevance of this, spanContextSupplier.getTraceId() is called in this if body, you modify the span for the same condition so this works as it should be, doesn't it?

So to me, if feels more solid to add the tagging behaviour close to ExemplarSampler. Here's how we are doing it (partially omitted for brevity):

The only thing I can think of right now is doing this in a getter (getTraceId) is not very elegant but getting the span twice (in the sampler) isn't either. I consider both kind of a hack until Prometheus will support this. One hack though is simpler to do because Boot backs off. Don't get me wrong, I still think your change is valid and we should merge it.

Let's continue this discussion here: prometheus/client_java#843, if we can find a good general solution, I think we can add support for this in Micrometer and Boot.

[snicoll]

@johnnywiller thank you for making your first contribution to Spring Boot.