Skip to content

Commit c9dad1f

Browse files
jonatan-ivanovjonatan-ivanov
committed
Fix logic to choose ObservationConvention
fixes gh-3543
1 parent 4647bf2 commit c9dad1f

6 files changed

Lines changed: 327 additions & 62 deletions

File tree

‎micrometer-observation-test/src/main/java/io/micrometer/observation/tck/ObservationRegistryCompatibilityKit.java‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -765,25 +765,20 @@ void observationFieldsShouldBeSetOnContext() {
765765
.highCardinalityKeyValue("hcTag1", "0")
766766
// should override the previous line
767767
.highCardinalityKeyValue("hcTag1", "3").highCardinalityKeyValues(KeyValues.of("hcTag2", "4"))
768-
.observationConvention(new TestObservationConvention("local"))
769-
.observationConvention(new UnsupportedObservationConvention("local"))
770768
.contextualName("test.observation.42").error(exception).start();
771769
observation.stop();
772770

773771
assertingHandler.checkAssertions(context -> {
774772
assertThat(context).isSameAs(testContext);
775773
assertThat(context.getName()).isEqualTo("test.observation");
776774
assertThat(context.getLowCardinalityKeyValues()).containsExactlyInAnyOrder(KeyValue.of("lcTag1", "1"),
777-
KeyValue.of("lcTag2", "2"), KeyValue.of("local.context.class", "TestContext"),
778-
KeyValue.of("global.context.class", "TestContext"));
775+
KeyValue.of("lcTag2", "2"), KeyValue.of("global.context.class", "TestContext"));
779776
assertThat(context.getHighCardinalityKeyValues()).containsExactlyInAnyOrder(KeyValue.of("hcTag1", "3"),
780-
KeyValue.of("hcTag2", "4"), KeyValue.of("local.uuid", testContext.uuid),
781-
KeyValue.of("global.uuid", testContext.uuid));
777+
KeyValue.of("hcTag2", "4"), KeyValue.of("global.uuid", testContext.uuid));
782778

783779
assertThat(context.getAllKeyValues()).containsExactlyInAnyOrder(KeyValue.of("lcTag1", "1"),
784-
KeyValue.of("lcTag2", "2"), KeyValue.of("local.context.class", "TestContext"),
785-
KeyValue.of("global.context.class", "TestContext"), KeyValue.of("hcTag1", "3"),
786-
KeyValue.of("hcTag2", "4"), KeyValue.of("local.uuid", testContext.uuid),
780+
KeyValue.of("lcTag2", "2"), KeyValue.of("global.context.class", "TestContext"),
781+
KeyValue.of("hcTag1", "3"), KeyValue.of("hcTag2", "4"),
787782
KeyValue.of("global.uuid", testContext.uuid));
788783

789784
assertThat((String) context.get("context.field")).isEqualTo("42");
@@ -795,9 +790,9 @@ void observationFieldsShouldBeSetOnContext() {
795790
.containsOnlyOnce("contextualName='test.observation.42'")
796791
.containsOnlyOnce("error='java.io.IOException: simulated'")
797792
.containsOnlyOnce(
798-
"lowCardinalityKeyValues=[global.context.class='TestContext', lcTag1='1', lcTag2='2', local.context.class='TestContext']")
799-
.containsOnlyOnce("highCardinalityKeyValues=[global.uuid='" + testContext.uuid
800-
+ "', hcTag1='3', hcTag2='4', local.uuid='" + testContext.uuid + "']")
793+
"lowCardinalityKeyValues=[global.context.class='TestContext', lcTag1='1', lcTag2='2']")
794+
.containsOnlyOnce(
795+
"highCardinalityKeyValues=[global.uuid='" + testContext.uuid + "', hcTag1='3', hcTag2='4']")
801796
.containsOnlyOnce("map=[context.field='42']");
802797
});
803798
}

‎micrometer-observation/src/main/java/io/micrometer/observation/Observation.java‎

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ static <T extends Context> Observation createNotStarted(String name, Supplier<T>
127127
return new SimpleObservation(name, registry, context == null ? new Context() : context);
128128
}
129129

130+
// @formatter:off
130131
/**
131132
* Creates but <b>does not start</b> an {@link Observation}. Remember to call
132133
* {@link Observation#start()} when you want the measurements to start. When the
@@ -143,15 +144,37 @@ static <T extends Context> Observation createNotStarted(String name, Supplier<T>
143144
* {@link ObservationRegistry.ObservationConfig#getObservationConvention(Context, ObservationConvention)})
144145
* was found. The {@link ObservationConvention} implementation can override
145146
* {@link Observation} names (i.e. name and contextual name) and key values.
147+
* <pre>
148+
* Here you can find the matrix of choosing the convention:
149+
* 1. custom default no pre-configured -> custom
150+
* 2. custom default pre-configured -> custom (not a valid case, just use custom)
151+
* 3. no custom default no pre-configured -> default
152+
* 4. no custom default pre-configured -> pre-configured
153+
* 5. custom no default no pre-configured -> custom (providing default is recommended)
154+
* 6. custom no default pre-configured -> custom (providing default is recommended)
155+
* 7. no custom no default no pre-configured -> local names/tags will be used
156+
* 8. no custom no default pre-configured -> pre-configured
157+
* </pre>
158+
* <p>
159+
* Also, if you set a convention using,
160+
* {@link Observation#observationConvention(ObservationConvention)} (not recommended),
161+
* the convention you set here will be used and everything else (custom, default,
162+
* pre-configured) will be ignored.
163+
* </p>
164+
* <p>
165+
* If you want to add to the all the contexts or mutate them,
166+
* use the ObservationFilter (e.g.: add region=cloud-1 or remove PII).
167+
* </p>
146168
* @param <T> type of context
147169
* @param customConvention custom convention. If {@code null}, the default one will be
148170
* picked.
149171
* @param defaultConvention default convention when no custom convention was passed,
150-
* nor a configured one was found
172+
* nor a pre-configured one was found
151173
* @param contextSupplier supplier for the observation context
152174
* @param registry observation registry
153175
* @return created but not started observation
154176
*/
177+
// @formatter:on
155178
static <T extends Context> Observation createNotStarted(@Nullable ObservationConvention<T> customConvention,
156179
ObservationConvention<T> defaultConvention, Supplier<T> contextSupplier, ObservationRegistry registry) {
157180
if (registry.isNoop()) {
@@ -174,6 +197,11 @@ static <T extends Context> Observation createNotStarted(@Nullable ObservationCon
174197
/**
175198
* Creates and starts an {@link Observation}. When no registry is passed or
176199
* observation is not applicable will return a no-op observation.
200+
* <p>
201+
* Please check the javadoc of
202+
* {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
203+
* method for the logic of choosing the convention.
204+
* </p>
177205
* @param observationConvention observation convention
178206
* @param registry observation registry
179207
* @return started observation
@@ -192,6 +220,11 @@ static Observation start(ObservationConvention<Context> observationConvention, O
192220
* {@link ObservationRegistry.ObservationConfig#observationPredicate(ObservationPredicate)
193221
* ObservationConfig#observationPredicate}), a no-op observation will also be
194222
* returned.
223+
* <p>
224+
* Please check the javadoc of
225+
* {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
226+
* method for the logic of choosing the convention.
227+
* </p>
195228
* @param <T> type of context
196229
* @param observationConvention observation convention
197230
* @param contextSupplier mutable context supplier
@@ -216,6 +249,11 @@ static <T extends Context> Observation start(ObservationConvention<T> observatio
216249
* provide a default one if neither a custom nor a pre-configured one (via
217250
* {@link ObservationRegistry.ObservationConfig#getObservationConvention(Context, ObservationConvention)})
218251
* was found.
252+
* <p>
253+
* Please check the javadoc of
254+
* {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
255+
* method for the logic of choosing the convention.
256+
* </p>
219257
* @param <T> type of context
220258
* @param registry observation registry
221259
* @param contextSupplier the observation context supplier
@@ -235,6 +273,11 @@ static <T extends Context> Observation start(@Nullable ObservationConvention<T>
235273
* {@link Observation#start()} when you want the measurements to start. When no
236274
* registry is passed or observation is not applicable will return a no-op
237275
* observation.
276+
* <p>
277+
* Please check the javadoc of
278+
* {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
279+
* method for the logic of choosing the convention.
280+
* </p>
238281
* @param observationConvention observation convention
239282
* @param registry observation registry
240283
* @return created but not started observation
@@ -265,6 +308,11 @@ static Observation createNotStarted(ObservationConvention<Context> observationCo
265308
* of {@link Context} to be passed and if you're not providing one we won't be able to
266309
* initialize it ourselves.
267310
* </p>
311+
* <p>
312+
* Please check the javadoc of
313+
* {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
314+
* method for the logic of choosing the convention.
315+
* </p>
268316
* @param <T> type of context
269317
* @param observationConvention observation convention
270318
* @param contextSupplier mutable context supplier
@@ -380,8 +428,8 @@ default boolean isNoop() {
380428

381429
/**
382430
* Adds an observation convention that can be used to attach key values to the
383-
* observation. WARNING: You must add ObservationConvention instances to the
384-
* Observation before it is started.
431+
* observation. WARNING: You must set the ObservationConvention to the Observation
432+
* before it is started.
385433
* @param observationConvention observation convention
386434
* @return this
387435
*/

‎micrometer-observation/src/main/java/io/micrometer/observation/ObservationRegistry.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.util.List;
2222
import java.util.Objects;
2323
import java.util.concurrent.CopyOnWriteArrayList;
24+
import java.util.function.Supplier;
2425

2526
/**
2627
* Implementations of this interface are responsible for managing state of an
@@ -135,6 +136,11 @@ public ObservationConfig observationFilter(ObservationFilter observationFilter)
135136

136137
/**
137138
* Register an {@link ObservationConvention}.
139+
* <p>
140+
* Please check the javadoc of
141+
* {@link Observation#createNotStarted(ObservationConvention, ObservationConvention, Supplier, ObservationRegistry)}
142+
* method for the logic of choosing the convention.
143+
* </p>
138144
* @param observationConvention observation convention
139145
* @return This configuration instance
140146
*/

‎micrometer-observation/src/main/java/io/micrometer/observation/SimpleObservation.java‎

Lines changed: 21 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,9 @@ class SimpleObservation implements Observation {
3838

3939
private final Context context;
4040

41+
@Nullable
4142
@SuppressWarnings("rawtypes")
42-
private final Collection<ObservationConvention> conventions;
43+
private ObservationConvention convention;
4344

4445
@SuppressWarnings("rawtypes")
4546
private final Deque<ObservationHandler> handlers;
@@ -50,27 +51,31 @@ class SimpleObservation implements Observation {
5051
this.registry = registry;
5152
this.context = context;
5253
this.context.setName(name);
53-
this.conventions = getConventionsFromConfig(registry, context);
54+
this.convention = getConventionFromConfig(registry, context);
5455
this.handlers = getHandlersFromConfig(registry, context);
5556
this.filters = registry.observationConfig().getObservationFilters();
5657
}
5758

5859
SimpleObservation(ObservationConvention<? extends Context> convention, ObservationRegistry registry,
5960
Context context) {
60-
this((String) null, registry, context); // name is set later in start()
61+
this.registry = registry;
62+
this.context = context;
63+
// name is set later in start()
64+
this.handlers = getHandlersFromConfig(registry, context);
65+
this.filters = registry.observationConfig().getObservationFilters();
6166
if (convention.supportsContext(context)) {
62-
this.conventions.add(convention);
67+
this.convention = convention;
6368
}
6469
else {
6570
throw new IllegalStateException(
6671
"Convention [" + convention + "] doesn't support context [" + context + "]");
6772
}
6873
}
6974

70-
private static Collection<ObservationConvention> getConventionsFromConfig(ObservationRegistry registry,
71-
Context context) {
75+
@Nullable
76+
private static ObservationConvention getConventionFromConfig(ObservationRegistry registry, Context context) {
7277
return registry.observationConfig().getObservationConventions().stream()
73-
.filter(convention -> convention.supportsContext(context)).collect(Collectors.toList());
78+
.filter(convention -> convention.supportsContext(context)).findFirst().orElse(null);
7479
}
7580

7681
private static Deque<ObservationHandler> getHandlersFromConfig(ObservationRegistry registry, Context context) {
@@ -105,7 +110,7 @@ public Observation highCardinalityKeyValue(KeyValue keyValue) {
105110
@Override
106111
public Observation observationConvention(ObservationConvention<?> convention) {
107112
if (convention.supportsContext(context)) {
108-
this.conventions.add(convention);
113+
this.convention = convention;
109114
}
110115
return this;
111116
}
@@ -125,18 +130,13 @@ public Observation event(Event event) {
125130

126131
@Override
127132
public Observation start() {
128-
// We want to rename with the first matching convention
129-
boolean nameChanged = false;
130-
for (ObservationConvention convention : this.conventions) {
133+
if (this.convention != null) {
131134
this.context.addLowCardinalityKeyValues(convention.getLowCardinalityKeyValues(context));
132135
this.context.addHighCardinalityKeyValues(convention.getHighCardinalityKeyValues(context));
133136

134-
if (!nameChanged) {
135-
String newName = convention.getName();
136-
if (StringUtils.isNotBlank(newName)) {
137-
this.context.setName(newName);
138-
nameChanged = true;
139-
}
137+
String newName = convention.getName();
138+
if (StringUtils.isNotBlank(newName)) {
139+
this.context.setName(newName);
140140
}
141141
}
142142

@@ -152,18 +152,13 @@ public Context getContext() {
152152
@SuppressWarnings({ "rawtypes", "unchecked" })
153153
@Override
154154
public void stop() {
155-
// We want to rename with the first matching convention
156-
boolean contextualNameChanged = false;
157-
for (ObservationConvention convention : this.conventions) {
155+
if (this.convention != null) {
158156
this.context.addLowCardinalityKeyValues(convention.getLowCardinalityKeyValues(context));
159157
this.context.addHighCardinalityKeyValues(convention.getHighCardinalityKeyValues(context));
160158

161-
if (!contextualNameChanged) {
162-
String newContextualName = convention.getContextualName(context);
163-
if (StringUtils.isNotBlank(newContextualName)) {
164-
this.context.setContextualName(newContextualName);
165-
contextualNameChanged = true;
166-
}
159+
String newContextualName = convention.getContextualName(context);
160+
if (StringUtils.isNotBlank(newContextualName)) {
161+
this.context.setContextualName(newContextualName);
167162
}
168163
}
169164

‎micrometer-observation/src/test/java/io/micrometer/observation/ObservationRegistryTest.java‎

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -96,8 +96,6 @@ void observationShouldWorkWithConventions() {
9696
registry.observationConfig().observationHandler(c -> true);
9797
// Define a convention
9898
MessagingConvention messagingConvention = new OurCompanyStandardMessagingConvention();
99-
// Register a semantic name provider
100-
registry.observationConfig().observationConvention(new OurCompanyObservationConvention());
10199

102100
Observation.Context myContext = new MessagingContext().put("foo", "hello");
103101
// Observation convention wants to use a MessagingConvention
@@ -125,6 +123,12 @@ static class MessagingObservationConvention implements ObservationConvention<Mes
125123
this.messagingConvention = messagingConvention;
126124
}
127125

126+
// Here we override the default "observation" name
127+
@Override
128+
public String getName() {
129+
return "new name";
130+
}
131+
128132
@Override
129133
public KeyValues getLowCardinalityKeyValues(MessagingContext context) {
130134
return KeyValues.of(this.messagingConvention.queueName(context.get("foo")));
@@ -153,22 +157,4 @@ public KeyValue queueName(String messagePayload) {
153157

154158
}
155159

156-
static class OurCompanyObservationConvention implements GlobalObservationConvention<Observation.Context> {
157-
158-
// Here we override the default "observation" name
159-
@Override
160-
public String getName() {
161-
return "new name";
162-
}
163-
164-
// This semantic name provider is only applicable when we're using a messaging
165-
// context
166-
167-
@Override
168-
public boolean supportsContext(Observation.Context context) {
169-
return context instanceof MessagingContext;
170-
}
171-
172-
}
173-
174160
}

0 commit comments

Comments
 (0)