Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take Supplier<Context> instead of Context #3443

Merged
merged 7 commits into from
Sep 30, 2022
Merged

Take Supplier<Context> instead of Context #3443

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5b298a2
WIP Take Supplier<Context> instead of Context
shakuzen Sep 29, 2022
e883514
Use method reference instead of lambda returning null
shakuzen Sep 30, 2022
f06a6c1
Polish
shakuzen Sep 30, 2022
4bbe048
Polish
shakuzen Sep 30, 2022
d8e73eb
Make sure context supplier only called once
shakuzen Sep 30, 2022
9d5ff0f
Remove wildcard generics where it doesn't make sense
shakuzen Sep 30, 2022
34b98a5
JavaDocs updated with an explanation of no-op behavior
shakuzen Sep 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...a/io/micrometer/core/instrument/binder/jersey/server/ObservationRequestEventListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public void onEvent(RequestEvent event) {
case REQUEST_MATCHED:
JerseyContext jerseyContext = new JerseyContext(event);
Observation observation = JerseyObservationDocumentation.DEFAULT.start(this.jerseyObservationConvention,
new DefaultJerseyObservationConvention(this.metricName), jerseyContext, this.registry);
new DefaultJerseyObservationConvention(this.metricName), () -> jerseyContext, this.registry);
Observation.Scope scope = observation.openScope();
observations.put(event.getContainerRequest(), new ObservationScopeAndContext(scope, jerseyContext));
break;
Expand Down
10 changes: 9 additions & 1 deletion ...ometer-core/src/main/java/io/micrometer/core/instrument/binder/okhttp3/OkHttpContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,17 @@

import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.function.Supplier;

/**
* A {@link SenderContext} for OkHttp3.
*
* @author Marcin Grzejszczak
* @since 1.10.0
*/
public class OkHttpContext extends RequestReplySenderContext<Request.Builder, Response> {
@SuppressWarnings("jol")
public class OkHttpContext extends RequestReplySenderContext<Request.Builder, Response>
implements Supplier<OkHttpContext> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


private final Function<Request, String> urlMapper;

Expand Down Expand Up @@ -95,4 +98,9 @@ public Request getOriginalRequest() {
return originalRequest;
}

@Override
public OkHttpContext get() {
return this;
}

}
5 changes: 3 additions & 2 deletions ...o/micrometer/core/instrument/observation/ObservationOrTimerCompatibleInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,14 @@ private ObservationOrTimerCompatibleInstrumentation(MeterRegistry meterRegistry,
this.defaultConvention = defaultConvention;
}

@SuppressWarnings("unchecked")
private void start(Supplier<T> contextSupplier) {
if (observationRegistry.isNoop()) {
timerSample = Timer.start(meterRegistry);
}
else {
context = contextSupplier.get();
observation = Observation.start(convention, defaultConvention, context, observationRegistry);
observation = Observation.start(convention, defaultConvention, contextSupplier, observationRegistry);
context = (T) observation.getContext();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

shakuzen marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
6 changes: 3 additions & 3 deletions ...test/src/main/java/io/micrometer/observation/tck/ObservationRegistryCompatibilityKit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ void observationFieldsShouldBeSetOnContext() {
TestContext testContext = new TestContext();
testContext.put("context.field", "42");
Exception exception = new IOException("simulated");
Observation observation = Observation.createNotStarted("test.observation", testContext, registry)
Observation observation = Observation.createNotStarted("test.observation", () -> testContext, registry)
.lowCardinalityKeyValue("lcTag1", "0")
// should override the previous line
.lowCardinalityKeyValue("lcTag1", "1").lowCardinalityKeyValues(KeyValues.of("lcTag2", "2"))
Expand Down Expand Up @@ -809,7 +809,7 @@ void globallyOverridenNameAndContextualNameShouldBeSetOnContext() {
.observationHandler(assertingHandler);

TestContext testContext = new TestContext();
Observation observation = Observation.createNotStarted("test.observation", testContext, registry)
Observation observation = Observation.createNotStarted("test.observation", () -> testContext, registry)
.contextualName("test.observation.42").start();
observation.stop();

Expand All @@ -829,7 +829,7 @@ void locallyOverridenNameAndContextualNameShouldBeSetOnContext() {
registry.observationConfig().observationHandler(assertingHandler);

TestContext testContext = new TestContext();
Observation observation = Observation.createNotStarted("test.observation", testContext, registry)
Observation observation = Observation.createNotStarted("test.observation", () -> testContext, registry)
.contextualName("test.observation.42")
.observationConvention(new TestObservationConventionWithNameOverrides()).start();
observation.stop();
Expand Down
15 changes: 13 additions & 2 deletions ...ation-test/src/test/java/io/micrometer/observation/tck/ObservationContextAssertTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import java.util.function.Supplier;

import static io.micrometer.observation.tck.ObservationContextAssert.assertThat;
import static org.assertj.core.api.BDDAssertions.thenNoException;
import static org.assertj.core.api.BDDAssertions.thenThrownBy;
Expand All @@ -30,13 +32,22 @@ class ObservationContextAssertTests {

ObservationRegistry registry;

Observation.Context context;
TestContext context;

static class TestContext extends Observation.Context implements Supplier<TestContext> {

@Override
public TestContext get() {
return this;
}

}

@BeforeEach
void beforeEach() {
registry = ObservationRegistry.create();
registry.observationConfig().observationHandler(c -> true);
context = new Observation.Context();
context = new TestContext();
}

@Test
Expand Down
67 changes: 37 additions & 30 deletions micrometer-observation/src/main/java/io/micrometer/observation/Observation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,20 @@ public interface Observation extends ObservationView {
* @return started observation
*/
static Observation start(String name, ObservationRegistry registry) {
return start(name, null, registry);
return start(name, Context::new, registry);
}

/**
* Creates and starts an {@link Observation}. When no registry is passed or
* observation is not applicable will return a no-op observation.
* @param name name of the observation
* @param context mutable context
* @param contextSupplier mutable context
* @param registry observation registry
* @return started observation
*/
static Observation start(String name, @Nullable Context context, @Nullable ObservationRegistry registry) {
return createNotStarted(name, context, registry).start();
static <T extends Context> Observation start(String name, Supplier<T> contextSupplier,
@Nullable ObservationRegistry registry) {
return createNotStarted(name, contextSupplier, registry).start();
}

/**
Expand All @@ -84,7 +85,7 @@ static Observation start(String name, @Nullable Context context, @Nullable Obser
* @return created but not started observation
*/
static Observation createNotStarted(String name, @Nullable ObservationRegistry registry) {
return createNotStarted(name, null, registry);
return createNotStarted(name, Context::new, registry);
}

/**
Expand All @@ -93,14 +94,17 @@ static Observation createNotStarted(String name, @Nullable ObservationRegistry r
* registry is passed or observation is not applicable will return a no-op
* observation.
* @param name name of the observation
* @param context mutable context
* @param contextSupplier supplier for mutable context
* @param registry observation registry
* @return created but not started observation
*/
static Observation createNotStarted(String name, @Nullable Context context,
static <T extends Context> Observation createNotStarted(String name, Supplier<T> contextSupplier,
@Nullable ObservationRegistry registry) {
if (registry == null || registry.isNoop()
|| !registry.observationConfig().isObservationEnabled(name, context)) {
if (registry == null || registry.isNoop()) {
return NOOP;
}
Context context = contextSupplier.get();
if (!registry.observationConfig().isObservationEnabled(name, context)) {
return NOOP;
}
return new SimpleObservation(name, registry, context == null ? new Context() : context);
Expand All @@ -120,21 +124,24 @@ static Observation createNotStarted(String name, @Nullable Context context,
* picked.
* @param defaultConvention default convention when no custom convention was passed,
* nor a configured one was found
* @param context the observation context
* @param contextSupplier supplier for the observation context
* @param registry observation registry
* @return created but not started observation
*/
static <T extends Context> Observation createNotStarted(@Nullable ObservationConvention<T> customConvention,
@NonNull ObservationConvention<T> defaultConvention, @NonNull T context,
@NonNull ObservationRegistry registry) {
ObservationConvention<T> defaultConvention, Supplier<T> contextSupplier, ObservationRegistry registry) {
if (registry.isNoop()) {
return Observation.NOOP;
}
ObservationConvention<T> convention;
if (customConvention != null) {
convention = customConvention;
}
else {
convention = registry.observationConfig().getObservationConvention(context, defaultConvention);
convention = registry.observationConfig().getObservationConvention(contextSupplier.get(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

defaultConvention);
}
return Observation.createNotStarted(convention, context, registry);
return Observation.createNotStarted(convention, contextSupplier, registry);
}

/**
Expand All @@ -146,21 +153,21 @@ static <T extends Context> Observation createNotStarted(@Nullable ObservationCon
*/
static Observation start(ObservationConvention<? extends Context> observationConvention,
@Nullable ObservationRegistry registry) {
return start(observationConvention, null, registry);
return start(observationConvention, () -> null, registry);
Copy link
Member Author

@shakuzen shakuzen Sep 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
* Creates and starts an {@link Observation}. When no registry is passed or
* observation is not applicable will return a no-op observation.
* @param <T> type of context
* @param observationConvention observation convention
* @param context mutable context
* @param contextSupplier mutable context
* @param registry observation registry
* @return started observation
*/
static <T extends Context> Observation start(ObservationConvention<T> observationConvention, @Nullable T context,
@Nullable ObservationRegistry registry) {
return createNotStarted(observationConvention, context, registry).start();
static <T extends Context> Observation start(ObservationConvention<T> observationConvention,
Supplier<T> contextSupplier, @Nullable ObservationRegistry registry) {
return createNotStarted(observationConvention, contextSupplier, registry).start();
}

/**
Expand All @@ -172,17 +179,16 @@ static <T extends Context> Observation start(ObservationConvention<T> observatio
* was found.
* @param <T> type of context
* @param registry observation registry
* @param context the observation context
* @param contextSupplier the observation context
* @param customConvention custom convention. If {@code null}, the default one will be
* picked.
* @param defaultConvention default convention when no custom convention was passed,
* nor a configured one was found
* @return started observation
*/
static <T extends Context> Observation start(@Nullable ObservationConvention<T> customConvention,
@NonNull ObservationConvention<T> defaultConvention, @NonNull T context,
@NonNull ObservationRegistry registry) {
return createNotStarted(customConvention, defaultConvention, context, registry).start();
ObservationConvention<T> defaultConvention, Supplier<T> contextSupplier, ObservationRegistry registry) {
return createNotStarted(customConvention, defaultConvention, contextSupplier, registry).start();
}

/**
Expand All @@ -196,7 +202,7 @@ static <T extends Context> Observation start(@Nullable ObservationConvention<T>
*/
static Observation createNotStarted(ObservationConvention<? extends Context> observationConvention,
@Nullable ObservationRegistry registry) {
return createNotStarted(observationConvention, null, registry);
return createNotStarted(observationConvention, () -> null, registry);
}

/**
Expand All @@ -215,18 +221,19 @@ static Observation createNotStarted(ObservationConvention<? extends Context> obs
* </p>
* @param <T> type of context
* @param observationConvention observation convention
* @param context mutable context
* @param contextSupplier mutable context
* @param registry observation registry
* @return created but not started observation
*/
static <T extends Context> Observation createNotStarted(ObservationConvention<T> observationConvention,
@Nullable T context, @Nullable ObservationRegistry registry) {
if (registry == null || registry.isNoop()
|| !registry.observationConfig().isObservationEnabled(observationConvention.getName(), context)
|| observationConvention == NoopObservationConvention.INSTANCE) {
Supplier<T> contextSupplier, @Nullable ObservationRegistry registry) {
if (registry == null || registry.isNoop() || observationConvention == NoopObservationConvention.INSTANCE) {
return NOOP;
}
T context = contextSupplier.get();
if (!registry.observationConfig().isObservationEnabled(observationConvention.getName(), context)) {
return NOOP;
}

return new SimpleObservation(observationConvention, registry, context == null ? new Context() : context);
}

Expand Down
2 changes: 1 addition & 1 deletion ...n/src/main/java/io/micrometer/observation/aop/ObservedAspectObservationDocumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ static Observation of(ProceedingJoinPoint pjp, Observed observed, ObservationReg
? signature.getDeclaringType().getSimpleName() + "#" + signature.getName() : observed.contextualName();

Observation observation = Observation
.createNotStarted(name, new ObservedAspect.ObservedAspectContext(pjp), registry)
.createNotStarted(name, () -> new ObservedAspect.ObservedAspectContext(pjp), registry)
.contextualName(contextualName)
.lowCardinalityKeyValue(CLASS_NAME.asString(), signature.getDeclaringTypeName())
.lowCardinalityKeyValue(METHOD_NAME.asString(), signature.getName())
Expand Down
Loading