Provide a way to fully reset the Metrics.globalRegistry

[youribonnaffe]

Metrics.globalRegistry works as a singleton and is often used across codebases as an easy way to register metrics.
While dependency injection might be preferred depends on your existing codebase and sometimes it gets difficult to work around it. Relying on singletons also seem to be the approach preferred by OpenTelemetry.

Now when it comes to testing one would like to fully reset the Metrics.globalRegistry so that previous metrics or configuration do not affect subsequent tests. Today meter registries and meters can be removed or cleared. For instance we developed a JUnit extension to do this work. However there is still state being kept up in the global registry such as:

• metric filters
• meter listeners
• naming convention
• pause detector

In my particular case metric filters have been problematic and there is no proper way to reset those. As a workaround we are using reflection to access the filters field.

While it would be difficult to reset this state with the existing clear method (semantics would be off, it would change its current meaning) maybe a new reset method could be introduced. I'm not sure about the impact of having this (and using it) on a live application but in the context of unit tests it would help.

Another alternative could be to provide a way to reset the Metrics.globalRegistry itself (making it writable) but that would probably not work with all use cases (if references are kept for instance).

If this sounds like an acceptable feature I can probably try to implement something although I'll be a bit worried about the handling of locks inside the MeterRegistry class and the impact of resetting state in case of a live application.

[cykl]

Related Slack discussion: https://micrometer-metrics.slack.com/archives/C662HUJC9/p1699279781463659

tl;dr: Adding a clearAndRemoveFilters method to MeterRegistry would address this need.

[lenin-jaganathan]

+1. But the biggest fear is implementing this for tests might bring inconsistent states in actual usages. Removing meter filters essentially invalidates all the meters contained in the Registry (the names/tags/distribution configs/ even accept-deny state).

[wakingrufus]

I also have this use case

[jonatan-ivanov]

Globals being hard to test is exactly the reason why Metrics.globalRegistry is not recommended. Please try to inject the registry to your components and try to avoid using globals.

In the above scenario there are two things you can do:

1. Add a SimpleMeterRegistry in @BeforeEach and remove this registry in @AfterEach, and register your filters (and make config changes) on the registered SimpleMeterRegistry instead of the global.
2. In case you have multiple registries, do the same with a CompositeMeterRegistry and add/remove that from the global.

[github-actions]

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

[github-actions]

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open.

[wakingrufus]

@jonatan-ivanov it was a bit harsh of github actions bot to consider the Christmas to new year's week as a week of inactivity :/
Anyway, to respond to your feedback, I do use SimpleMeterRegistry 99.9% of the time in tests. However, my framework has a feature where it sets up the global registry just in case someone or some library uses it instead of properly injecting a registry. So it is in testing that type of code where this feature would be useful.

[shakuzen]

@wakingrufus we can always re-open the issue, so don't mind the bot closing it over the holidays too much. As for the feedback, @jonatan-ivanov provided a way that you can deal with testing usage of the global registry without needing a reset, because you can add and remove a SimpleMeterRegistry to the global registry and assert on it; so you have a fresh registry instance at whatever level you want in your tests. Here's the idea in a unit test:

class GlobalRegistryTest {

    MeterRegistry meterRegistry;

    @BeforeEach
    void setUp() {
        meterRegistry = new SimpleMeterRegistry();
        configureRegistry(meterRegistry);
        Metrics.globalRegistry.add(meterRegistry);
    }

    void configureRegistry(MeterRegistry meterRegistry) {
        meterRegistry.config().commonTags("test", "value");
    }

    @AfterEach
    void tearDown() {
        Metrics.globalRegistry.remove(meterRegistry);
    }

    @Test
    void oneStep() {
        codeThatUsesGlobalRegistry(1);
        assertThat(meterRegistry.get("steps").tag("test", "value").counter().count()).isEqualTo(1);
    }

    @Test
    void twoSteps() {
        codeThatUsesGlobalRegistry(2);
        assertThat(meterRegistry.get("steps").tag("test", "value").counter().count()).isEqualTo(2);
    }

    void codeThatUsesGlobalRegistry(int steps) {
        Metrics.counter("steps").increment(steps);
    }
}

In a different test class, you can configure the registry asserted against differently. Is there a reason this approach wouldn't work for your use case?

[youribonnaffe]

While I agree globals like Metrics.globalRegistry make tests harder to write and injecting a meter registry might be preferred, for such pervasive concerns, like metrics, logs, tracing, the global sometimes make it easier to use.

Passing around a registry in every class, just for the sake of injecting it in the right place is often making code a bit too verbose. You end up with classes that take in a constructor a metric registry, a clock, a tracing registry and so on, that makes it harder to work out what's the real business logic of the class.

For instance it is quite accepted that for loggers you refer to them statically where ever you need one and don't bother injecting them all along. Yet testing loggers is often a mess because logging frameworks are beasts with complex initializations but there is probably a way to make them testable with a few static helpers.

Time, for instance, was quite easy to control in tests back then with Joda and DateTimeUtils. Whereas the java 8 Clock makes it (arguably cleaner) but harder to use IMHO.

With regard to the last suggestion, I may have lost bits of what I was trying to do but if I remember correctly the issue we had was that some state was preserved in the global registry and not necessarily attached to a registry you could add or remove in your tests.

[shakuzen]

some state was preserved in the global registry and not necessarily attached to a registry you could add or remove in your tests.

If you configure something on the global registry, I would expect that to be the case. But if you are trying to test the effects of a specific metrics configuration in an isolated test, you should instead of configuring that on the global, configure it on a non-global instance that you add to the global, as demonstrated in my code above. What is changing the config of the global registry?

[cykl]

We discussed this on Slack a while ago. Link is in the second comment of this thread. Let's recap here:

• For unit tests that only create and use some meter, the suggestion made by Micrometer's maintainers works. It may be be annoying for user to have to write their own Junit Jupiter extension (or above suggested @(Before|After) rules) but it's not a big deal.
• For unit tests that set some MeterFilter to the global registry there is no way to reset those filters afterwards. This occurred to me while writing unit tests where the unit is a module or a microservice (what is named integration tests in https://engineering.atspotify.com/2018/01/testing-of-microservices/) because some initialization code configured the global registry as application code is expected to use the global registry. It was a deliberate choice to execute this initialization code because we think it's relevant to the test suite. I had to use reflection to be able to reset Micrometer which has obvious drawbacks.
• There seem to be different expectations between some users and Micrometer's maintainers. Maintainers are strongly in favor of DI for metrics, while some users find it annoying. Personally, I fully agree with @youribonnaffe regarding the injection of a MeterRegistry. While I understand when or where it can be beneficial, most of the time injecting a MeterRegistry is an annoyance and the global registry is super good enough. That's also how most observability libraries works.
• Micrometer provides Metrics#globalRegistry. Providing it but denying its relevance probably doesn't provide a clear message.

Consensus was that we could add a clearAndRemoveFilters function to the Metrics class. IIRC I started working on that but stopped after a couple of minutes because it was not as obvious as expected and I never got back into it.

[jonatan-ivanov]

For unit tests that set some MeterFilter to the global registry there is no way to reset those filters afterwards. [...]

That's we we recommended not to set those on the global for unit tests. If you have an integration test, you are in the exact scenario why we don't recommend using the global at all (because it is global per classloader). In that case, I think there are two things you can do: 1. Start the app from scratch for these tests and run a fresh instance of the app with freshly loaded classes (re-initialized global)
2. Do the same as we recommended for the unit tests: configure the registry instance you add to the global, not the global itself. You can do it in the separate test profile and in production too.

Does this work for you?

Micrometer provides Metrics#globalRegistry. Providing it but denying its relevance probably doesn't provide a clear message.

The Metrics class was created back in 2017, removing it would be a breaking change which we should only do in a major version but we can fix this in our docs site and in javadocs, we can also deprecate the Metrics class, what do you think?

[shakuzen]

I think it hasn't been stated in this thread, but the primary reservation among the maintainers with "just add a clearAndRemoveFilters method to MeterRegistry" is that it's a method only needed for testing but it would be there for users to wrongly use in production, causing misunderstandings and support issues. It's also not just filters, as shown in the description of this issue. I think we'd be more okay to go that route with something that was clearly for testing. Maybe we could offer a JUnit Jupiter extension - I haven't thought that fully through but that sounds more sustainable to me. Would that work for folks wanting this feature?

[wakingrufus]

I think it hasn't been stated in this thread, but the primary reservation among the maintainers with "just add a clearAndRemoveFilters method to MeterRegistry" is that it's a method only needed for testing but it would be there for users to wrongly use in production, causing misunderstandings and support issues. It's also not just filters, as shown in the description of this issue. I think we'd be more okay to go that route with something that was clearly for testing. Maybe we could offer a JUnit Jupiter extension - I haven't thought that fully through but that sounds more sustainable to me. Would that work for folks wanting this feature?

Yes that would work for me

[shakuzen]

Re-opening to explore the JUnit Jupiter extension idea.

[jonatan-ivanov]

I like the JUnit extension idea (/limiting the feature to tests), also as an alternative (or addition), we can add some kind of a "configurer" class to micrometer-test that provides one-liners to be used from @BeforeEach/@AfterEach or similar methods in other test libraries (like testNG).

Though, I would like to mention that depending on how we implement this, it might be still problematic (right now I'm not sure how to implement it in a way so that it cannot cause issues):

We can have a package-private reset method in Metrics that reinitializes the global static field (we need to make it non-final). We can call Metrics.reset() from a public static method that is only available in micrometer-test, so in your tests you would do something like this:

@AfterEach
void tearDown() {
    MetricsTestConfigurer.resetGlobalRegistry();
}

This is neat but it can cause issues at places that hold a reference to the old instance.

Another way to deal with this if we maintain a static composite and add/remove it to the global. This will fix the previous issue but nothing prevents the user to still configure against the global and the amount of code you need is pretty much the same as here: #4187 (comment), for comparison:

@BeforeEach
void setUp() {
    meterRegistry = new SimpleMeterRegistry();
    MetricsTestConfigurer.testRegistry.add(meterRegistry);
    MetricsTestConfigurer.testRegistry.config().commonTags("test", "value");
    MetricsTestConfigurer.addTestRegistryToGlobal(); // this is just Metrics.addRegistry(testRegistry);
}

@AfterEach
void tearDown() {
    MetricsTestConfigurer.removeTestRegistryFromGlobal(); // this is just Metrics.removeRegistry(testRegistry);
}

So could you please show us code examples about what you are doing so that we can see if "fixing" this would actually fix the issue or it won't since we are dealing with a static global no matter what?

Also, if anyone has any other idea how to do this in a safe way other than the two methods above, please let us know (and other than not using the global).

[youribonnaffe]

Extracting the code in a meaningful example would take a bit of time but let me try to explain our use case and see if it makes sense.

We have classes using the global registry covered by unit tests that will add a SimpleMeterRegistry to the global registry in the test, remove it after the test. Those works fine.

We also have more complex tests, loading the entire application or most of it. By doing so they also load common configuration classes that are adding meter filters to the global registry. Those tests can run fine on their own too.

When you mix both of them (i.e run all the tests of the application), then if the complex tests run first, filters will be added to the global registry and the behavior of the simpler tests will be affected, because said filters cannot be removed.

Having a test utility to clear the global registry like MetricsTestConfigurer.resetGlobalRegistry(); would likely work and prevent us from using reflection to do it.

This is neat but it can cause issues at places that hold a reference to the old instance.

Wouldn't this utility change the existing instance of the global registry, rather than re-assigning it?
In our code base I found places where references are kept on the global registry, however I'm not sure they are valid or problematic usages. But that is indeed hard to control.

[wakingrufus]

I came up with this Kotlin extension function as a workaround:

fun MeterRegistry.clearFilters() {
    val filtersField = MeterRegistry::class.java.getDeclaredField("filters")
    filtersField.setAccessible(true)
    filtersField.set(this, arrayOf<MeterFilter>())
}

so a static method in a test package that does something like

static void clearFilters(MeterRegistry meterRegistry){
        final var filtersField = MeterRegistry.class.getDeclaredField("filters");
        filtersField.setAccessible(true);
        filtersField.set(meterRegistry, new MeterFilter[]{});
    }

would be good enough I think, and probably less risky than going nuclear and replacing the whole static Composite registry reference