Skip to content

MeterRegistry implements ObservationRegistry, static factory methods #2

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

Merged
jonatan-ivanov merged 1 commit into from
Feb 3, 2022
Merged

MeterRegistry implements ObservationRegistry, static factory methods #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 29 additions & 1 deletion micrometer-api/src/main/java/io/micrometer/api/instrument/MeterRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@
import io.micrometer.api.instrument.noop.NoopMeter;
import io.micrometer.api.instrument.noop.NoopTimeGauge;
import io.micrometer.api.instrument.noop.NoopTimer;
import io.micrometer.api.instrument.observation.Observation;
import io.micrometer.api.instrument.observation.ObservationRegistry;
import io.micrometer.api.instrument.observation.TimerObservationHandler;
import io.micrometer.api.instrument.search.MeterNotFoundException;
import io.micrometer.api.instrument.search.RequiredSearch;
import io.micrometer.api.instrument.search.Search;
Expand Down Expand Up @@ -75,7 +78,7 @@
* @author Jon Schneider
* @author Johnny Lim
*/
public abstract class MeterRegistry {
public abstract class MeterRegistry implements ObservationRegistry {
protected final Clock clock;
private final Object meterMapLock = new Object();
private volatile MeterFilter[] filters = new MeterFilter[0];
Expand All @@ -85,6 +88,31 @@ public abstract class MeterRegistry {
private final Config config = new Config();
private final More more = new More();

private final ThreadLocal<Observation> localObservation = new ThreadLocal<>();

private final ObservationConfig observationConfig = new ObservationConfig();

@Nullable
@Override
public Observation getCurrentObservation() {
return this.localObservation.get();
}

@Override
public void setCurrentObservation(@Nullable Observation current) {
this.localObservation.set(current);
}

@Override
public ObservationConfig observationConfig() {
return this.observationConfig;
}

//TODO Want this under observationConfig but not sure that's possible
public void withTimerObservationHandler() {
Copy link
Owner Author

@jonatan-ivanov jonatan-ivanov Feb 2, 2022

Choose a reason for hiding this comment

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

Shouldn't this return ObservationConfig so that you can do this:

registry
    .withTimerObservationHandler()
    .observationHandler(new SampleHandler())
    .observationPredicate(...)

Copy link

@shakuzen shakuzen Feb 3, 2022

Choose a reason for hiding this comment

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

Ideally, yes. That was my intention, but I didn't see a practical way to do that without making it a method on ObservationConfig, which shouldn't directly know about Timer. I couldn't think of a way to add that only in MeterRegistry. We should definitely think of something better than what's there now. It was temporary since it was late and I was out of ideas but just wanted to demonstrate the general intention.

Copy link

@shakuzen shakuzen Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

To achieve what you mentioned, it works well enough to just return observationConfig after the current logic, but what I meant was that ideally the method itself would be nested under MeterRegistry#observationConfig() instead of a top-level method on MeterRegistry, I think. But that doesn't feel possible. So the question is perhaps are we okay with this (awkwardly) being a top-level method on MeterRegistry that returns the ObservationConfig? Another option is to put it under Config, but then that would be weird to return ObservationConfig from there.

observationConfig().observationHandler(new TimerObservationHandler(this));
}

// Even though writes are guarded by meterMapLock, iterators across value space are supported
// Hence, we use CHM to support that iteration without ConcurrentModificationException risk
private final Map<Id, Meter> meterMap = new ConcurrentHashMap<>();
Expand Down
28 changes: 28 additions & 0 deletions micrometer-api/src/main/java/io/micrometer/api/instrument/observation/Observation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Set;
import java.util.function.Function;

import io.micrometer.api.instrument.NoopObservation;
import io.micrometer.api.instrument.Tag;
import io.micrometer.api.instrument.Tags;
import io.micrometer.api.instrument.TagsProvider;
Expand All @@ -37,6 +38,33 @@
*/
public interface Observation {

static Observation start(String name, ObservationRegistry registry) {
return start(name, null, registry);
}

static Observation start(String name, @Nullable Context context, ObservationRegistry registry) {
return createNotStarted(name, context, registry).start();
}

/**
* !!!!!!!!!!!!!!!!!!!! THIS IS NOT STARTED !!!!!!!!!!!!!!!!!!!!
* !!!!!!!!!!!!!!!!!!!! REMEMBER TO CALL START() OTHERWISE YOU WILL FILE ISSUES THAT STUFF IS NOT WORKING !!!!!!!!!!!!!!!!!!!!
*/
static Observation createNotStarted(String name, ObservationRegistry registry) {
return createNotStarted(name, null, registry);
}

/**
* !!!!!!!!!!!!!!!!!!!! THIS IS NOT STARTED !!!!!!!!!!!!!!!!!!!!
* !!!!!!!!!!!!!!!!!!!! REMEMBER TO CALL START() OTHERWISE YOU WILL FILE ISSUES THAT STUFF IS NOT WORKING !!!!!!!!!!!!!!!!!!!!
*/
static Observation createNotStarted(String name, @Nullable Context context, ObservationRegistry registry) {
if (!registry.observationConfig().isObservationEnabled(name, context)) {
return NoopObservation.INSTANCE;
}
return new SimpleObservation(name, registry, context == null ? new Context() : context);
}

/**
* Sets the display name (a more human-readable name).
*
Expand Down
58 changes: 4 additions & 54 deletions ...meter-api/src/main/java/io/micrometer/api/instrument/observation/ObservationRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,60 +31,12 @@ public interface ObservationRegistry {

void setCurrentObservation(@Nullable Observation current);

/**
* !!!!!!!!!!!!!!!!!!!! THIS IS NOT STARTED !!!!!!!!!!!!!!!!!!!!
* !!!!!!!!!!!!!!!!!!!! REMEMBER TO CALL START() OTHERWISE YOU WILL FILE ISSUES THAT STUFF IS NOT WORKING !!!!!!!!!!!!!!!!!!!!
* @param name observation name
* @return this
*/
default Observation observation(String name) {
if (config().isObservationEnabled(name, null)) {
return new SimpleObservation(name, this, new Observation.Context());
}
return NoopObservation.INSTANCE;
}

/**
* !!!!!!!!!!!!!!!!!!!! THIS IS NOT STARTED !!!!!!!!!!!!!!!!!!!!
* !!!!!!!!!!!!!!!!!!!! REMEMBER TO CALL START() OTHERWISE YOU WILL FILE ISSUES THAT STUFF IS NOT WORKING !!!!!!!!!!!!!!!!!!!!
* @param name observation name
* @param context context
* @return this
*/
default Observation observation(String name, Observation.Context context) {
Copy link
Owner Author

@jonatan-ivanov jonatan-ivanov Feb 2, 2022

Choose a reason for hiding this comment

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

Should not we leave these so that you can do registry.observation(...) or Observation.createNotStarted(...)? Meters are similar from this perspective: registry.timer(...) and Timer.start(...).

Copy link

@shakuzen shakuzen Feb 3, 2022

Choose a reason for hiding this comment

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

Good question. I was mirroring the API after Sample usage since Observation like Sample is not a Meter. While there is registry.timer(...) and Timer.start(...), the former returns (and registers) a Timer while the latter doesn't register anything and returns a Sample. So I thought it would be better to have only the latter. My hope was also that it makes it more clear that it's not registering or giving you a Meter because it's not a method on MeterRegistry. What do you think?

if (config().isObservationEnabled(name, context)) {
return new SimpleObservation(name, this, context);
}
return NoopObservation.INSTANCE;
}

/**
* Started observation.
*
* @param name observation name
* @return this
*/
default Observation start(String name) {
return observation(name).start();
}

/**
* Started observation.
*
* @param name observation name
* @param context context
* @return this
*/
default Observation start(String name, Observation.Context context) {
return observation(name, context).start();
}

Config config();
ObservationConfig observationConfig();

/**
* Access to configuration options for this registry.
*/
class Config {
class ObservationConfig {

private List<ObservationHandler<?>> observationHandlers = new CopyOnWriteArrayList<>();

Expand All @@ -95,9 +47,8 @@ class Config {
*
* @param handler handler to add to the current configuration
* @return This configuration instance
* @since 2.0.0
*/
public ObservationRegistry.Config observationHandler(ObservationHandler<?> handler) {
public ObservationConfig observationHandler(ObservationHandler<?> handler) {
this.observationHandlers.add(handler);
return this;
}
Expand All @@ -108,9 +59,8 @@ public ObservationRegistry.Config observationHandler(ObservationHandler<?> handl
*
* @param predicate predicate
* @return This configuration instance
* @since 2.0.0
*/
public ObservationRegistry.Config observationPredicate(BiPredicate<String, Observation.Context> predicate) {
public ObservationConfig observationPredicate(BiPredicate<String, Observation.Context> predicate) {
this.observationPredicates.add(predicate);
return this;
}
Expand Down
11 changes: 4 additions & 7 deletions micrometer-api/src/main/java/io/micrometer/api/instrument/observation/SimpleObservation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,20 +22,17 @@
import io.micrometer.api.instrument.Tag;
import io.micrometer.api.lang.Nullable;

public class SimpleObservation implements Observation {
class SimpleObservation implements Observation {
private final ObservationRegistry registry;
private final Context context;
@SuppressWarnings("rawtypes")
private final Deque<ObservationHandler> handlers;

public SimpleObservation(String name, ObservationRegistry registry) {
this(name, registry, new Context());
}

public SimpleObservation(String name, ObservationRegistry registry, Context context) {
// package private so only instantiated by us
SimpleObservation(String name, ObservationRegistry registry, Context context) {
this.registry = registry;
this.context = context.setName(name);
this.handlers = registry.config().getObservationHandlers().stream()
this.handlers = registry.observationConfig().getObservationHandlers().stream()
.filter(handler -> handler.supportsContext(this.context))
.collect(Collectors.toCollection(ArrayDeque::new));
}
Expand Down
43 changes: 0 additions & 43 deletions ...api/src/main/java/io/micrometer/api/instrument/observation/SimpleObservationRegistry.java
View file
Open in desktop

This file was deleted.

27 changes: 0 additions & 27 deletions ...-api/src/main/java/io/micrometer/api/instrument/observation/TimerObservationRegistry.java
View file
Open in desktop

This file was deleted.

5 changes: 3 additions & 2 deletions ...micrometer/api/instrument/observation/AllMatchingCompositeTimerRecordingHandlerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package io.micrometer.api.instrument.observation;

import io.micrometer.api.instrument.observation.ObservationHandler.AllMatchingCompositeObservationHandler;
import io.micrometer.api.instrument.simple.SimpleMeterRegistry;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -26,9 +27,9 @@ class AllMatchingCompositeTimerRecordingHandlerTests {

MatchingHandler matchingHandler2 = new MatchingHandler();

ObservationRegistry registry = new SimpleObservationRegistry();
ObservationRegistry registry = new SimpleMeterRegistry();

Observation sample = registry.start("hello");
Observation sample = Observation.start("hello", registry);

@Test
void should_run_on_start_for_all_matching_handlers() {
Expand Down
13 changes: 7 additions & 6 deletions ...er-api/src/test/java/io/micrometer/api/instrument/observation/CurrentObservationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
package io.micrometer.api.instrument.observation;

import io.micrometer.api.instrument.simple.SimpleMeterRegistry;
import org.junit.jupiter.api.Test;

import java.util.HashMap;
Expand All @@ -24,13 +25,13 @@
import static org.assertj.core.api.Assertions.assertThat;

class CurrentObservationTest {
private final ObservationRegistry registry = new SimpleObservationRegistry();
private final ObservationRegistry registry = new SimpleMeterRegistry();

@Test
void nestedSamples_parentChildThreadsInstrumented() throws ExecutionException, InterruptedException {
ExecutorService taskRunner = Executors.newSingleThreadExecutor();

Observation observation = registry.observation("test.observation");
Observation observation = Observation.createNotStarted("test.observation", registry);
System.out.println("Outside task: " + observation);
assertThat(registry.getCurrentObservation()).isNull();
try (Observation.Scope scope = observation.openScope()) {
Expand All @@ -48,7 +49,7 @@ void nestedSamples_parentChildThreadsInstrumented() throws ExecutionException, I
void start_thenStopOnChildThread() throws InterruptedException, ExecutionException {
ExecutorService executor = Executors.newSingleThreadExecutor();

Observation observation = registry.observation("test.observation");
Observation observation = Observation.createNotStarted("test.observation", registry);
assertThat(registry.getCurrentObservation()).isNull();
executor.submit(() -> {
try (Observation.Scope scope = observation.openScope()) {
Expand All @@ -68,7 +69,7 @@ void startOnChildThread_thenStopOnSiblingThread() throws InterruptedException, E
Map<String, Observation> observationMap = new HashMap<>();

executor.submit(() -> {
Observation observation = registry.observation("test.observation");
Observation observation = Observation.createNotStarted("test.observation", registry);
assertThat(registry.getCurrentObservation()).isNull();
observationMap.put("myObservation", observation);
}).get();
Expand All @@ -87,11 +88,11 @@ void startOnChildThread_thenStopOnSiblingThread() throws InterruptedException, E

@Test
void nestedSamples_sameThread() {
Observation observation = registry.observation("observation1");
Observation observation = Observation.createNotStarted("observation1", registry);
Observation observation2;
assertThat(registry.getCurrentObservation()).isNull();
try (Observation.Scope scope = observation.openScope()) {
observation2 = registry.observation("observation2");
observation2 = Observation.createNotStarted("observation2", registry);
assertThat(registry.getCurrentObservation()).isSameAs(observation);
}
try (Observation.Scope scope = observation2.openScope()) {
Expand Down
5 changes: 3 additions & 2 deletions .../micrometer/api/instrument/observation/FirstMatchingCompositeObservationHandlerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package io.micrometer.api.instrument.observation;

import io.micrometer.api.instrument.observation.ObservationHandler.FirstMatchingCompositeObservationHandler;
import io.micrometer.api.instrument.simple.SimpleMeterRegistry;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;
Expand All @@ -24,9 +25,9 @@ class FirstMatchingCompositeObservationHandlerTests {

MatchingHandler matchingHandler = new MatchingHandler();

ObservationRegistry registry = new SimpleObservationRegistry();
ObservationRegistry registry = new SimpleMeterRegistry();

Observation sample = registry.start("hello");
Observation sample = Observation.start("hello", registry);

@Test
void should_run_on_start_only_for_first_matching_handler() {
Expand Down
Loading