Take Supplier<Context> instead of Context

[shakuzen]

This allows avoiding instantiation of a Context object in the case that the ObservationRegistry is a no-op. This does not overload the methods by having a variant that takes a Context and one that takes a Supplier because this becomes ambiguous for the compiler in some cases (reported to us when we added one such method to ObservationDocumentation previously). A lambda can be used in cases where you have a Context object to pass. Alternatively, you can implement Supplier in your custom Context class to avoid needing a lambda.

Migration guide

Anywhere you were passing a Context object to an Observation factory method (static methods on Observation and instance methods on ObservationDocumentation), the parameter type has been changed to Supplier<Context> to allow for lazy instantiation of the context. There are a few options available on how to migrate your code.

Lambda

Using a lambda is one way to adapt code to the new API. Before where you may have had code like the following:

JerseyContext jerseyContext = new JerseyContext(event);
Observation observation = JerseyObservationDocumentation.DEFAULT.start(this.jerseyObservationConvention,
                        new DefaultJerseyObservationConvention(this.metricName), jerseyContext, this.registry);

It can be updated to:

JerseyContext jerseyContext = new JerseyContext(event);
Observation observation = JerseyObservationDocumentation.DEFAULT.start(this.jerseyObservationConvention,
                        new DefaultJerseyObservationConvention(this.metricName), () -> jerseyContext, this.registry);

Method reference

If you do not need to pass arguments to the context, you can use a method reference to the constructor.

Observation.start("my.observation", Context::new, observationRegistry)

Make your context a supplier

Another alternative if you are using a custom context is to make it implement Supplier and then you can pass the context object as you were before. For example, an OkHttpContext class could be updated to the following.

public class OkHttpContext extends RequestReplySenderContext<Request.Builder, Response>
        implements Supplier<OkHttpContext> {
    // ...
    @Override
    public OkHttpContext get() {
        return this;
    }

And then used the same as before, such as:

Observation observation = OkHttpObservationDocumentation.DEFAULT.observation(this.observationConvention,
        new DefaultOkHttpObservationConvention(requestMetricName), okHttpContext, this.registry).start();

[shakuzen]

Switched the order here so the supplier isn't called twice.

[shakuzen]

Demonstrates one way to deal with this breaking change for users. Then they can pass the, in this case, OkHttpContext instead of using a lambda.

[shakuzen]

This will result in the supplier being called twice, which feels less than ideal.

[jonatan-ivanov]

Can we review the noop checks for the whole class, probably refactor it into a method (or two) and do the registry based check early?
After we passed the registry based check, we can create the context once and pass it to the next method (a private overload without the supplier)?

[shakuzen]

One issue is interfaces can't have private methods. But regardless of that, it's a little hard to centralize this much more than it is, I think, due to the variety of factory methods and the different logic they have. Suggestions welcome, though.

[ttddyy]

What if we create a dedicated factory/builder class instead of default/static factory methods? The class would centralize noop checks for all variants of observation creations if possible.

[shakuzen]

Looking at this more, we have two constructors for SimpleObservation, so I think we're going to have at least two places in factory methods we need to check for a no-op registry. It turns out we end up with 3 now because there is the option to make an Observation with a name, with a convention, or to have the convention looked up. The convention look-up requires the context, so we need to get the context already at that point. I've refactored this method to not call one of the other methods and instead instantiate the SimpleObservation itself, which avoids supplying the context twice. It leads to a little code duplication, but the look-up of the convention in the registry is a notable difference that happens in the middle of the logic.

On a dedicated factory/builder class, that's an option. I worry it's a bigger change and we're basically out of time to iterate on and get feedback on such things. I don't know that it would change a lot. The no-op check that can be centralized I think is just calling registry.isNoop() and separately !registry.observationConfig().isObservationEnabled(name, context). This is already only done in the 3 places I mentioned above because they are end up doing different things.

A tangential thing is, we're not consistent on whether we accept a @Nullable ObservationRegistry or not in the factory methods. We can look at that separately, I think, but we should make that consistent unless there's some reason to be different.

[shakuzen]

Unrelated to this change, but I removed the @NonNull annotations because they are redundant. We annotate the whole package with @NonNullApi.

[sonatype-lift]

⚠️ 10 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

[jonatan-ivanov]

Should we provide a null context supplier so users can do something like this: start(name, Context.NULL, registry)?

Though if they want to pass null, they can just call the overload where this param is missing.

[shakuzen]

Yeah, I'm not sure what is best. It would seem a bit silly to provide a null supplier, which is just going to result in us eventually instantiating a Context object, rather than providing a default context supplier that returns a new Context().

[shakuzen]

I tried adding static supplier field to Context, but it doesn't really save anything compared to using Context::new instead, so I think I'll leave it out until/unless we get feedback from users. I'll switch these () -> null to Context::new instead.

[jonatan-ivanov]

Can we review the noop checks for the whole class, probably refactor it into a method (or two) and do the registry based check early?
After we passed the registry based check, we can create the context once and pass it to the next method (a private overload without the supplier)?

[jonatan-ivanov]

I quite like this actually. :D

[shakuzen]

I couldn't get rid of this () -> null lambda because of generics. I wonder if we need this method. What's the use case to provide an ObservationConvention that doesn't target Context and not provide a context? If it only ever targets Context, then we don't need the ? extends Context and then we can change the lambda to Context::new.
So unless I'm missing the use case, I think we should either delete this method, or change the first parameter type to ObservationConvention<Context> observationConvention. Thoughts?

[shakuzen]

I'll update the parameter type to ObservationConvention<Context> since having another Context type when a Context will be created doesn't make sense.