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

Jump to bottom

Removing registry from CompositeMeterRegistry removes composite meters added to the removed registry #2281

Open

admoca60 wants to merge 5 commits into micrometer-metrics:1.5.x from admoca60:bugfix/issue-1159

admoca60 commented Oct 7, 2020

This is the pull request to fix the issue #1159

My branch was created from 1.5.x. I don't know if it's correct or now. Please, fix me if I'm wrong!

Thank you!

Adrián Molina Calvo added 2 commits

October 7, 2020 12:31


          Create test to fix the bug

48274c7


          Fix issue micrometer-metrics#1159 to remove child meters

488b12d

checketts changed the title ~~Bugfix/issue 1159~~ Bugfix/issue 1159 (Removing registry from CompositeMeterRegistry)

Adrián Molina Calvo added 3 commits

October 8, 2020 12:47


          Keeping the references in cases of CompositeCustomMeter. This class d…

594156a

…oesn't extends AbstractCompositeMeter.


          Fix test to take into account that child meters references are remove…

fd1ffdd

…d when a child registry is removed


          Fixing checkstyle

93e343c

Author

admoca60 commented Oct 8, 2020

Currently, the PR fails because the test "io.micrometer.core.instrument.binder.logging.Log4j2MetricsTest > asyncLogShouldNotBeDuplicated()" fails.
This test doesn't fail in local environment and theorically, it isn't related to the changes in this PR.

Please, can anyone reRun the build in circleci? Thanks!

Author

admoca60 commented Oct 13, 2020

@jkschneider, could you help me? Thanks!

admoca60 mentioned this pull request

Can't remove meters after a filter is added #1597

Closed

shakuzen requested changes

View reviewed changes

Member

shakuzen left a comment

Thank you for the pull request and patience on it getting reviewed. I'd like to focus this pull request on solving the scenario for #1159. That is, removing a child registry from a composite should remove the meter contributed from the composite. I've handled fixing #2354 in a different way in #2362. The changes here now should work as is, but I've left some comments about simplifications given #2362, and the tests should be updated to ensure we're covering the original issue described in #1159.

Also, we will want to merge this into 1.3.x so we can include it in that line of maintenance releases, but I can take care of that on merging if you don't want to deal with rebasing and changing the target branch.

...meter-core/src/main/java/io/micrometer/core/instrument/composite/CompositeMeterRegistry.java Show resolved Hide resolved

...r-core/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java Show resolved Hide resolved

...r-core/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java Show resolved Hide resolved

micrometer-core/src/main/java/io/micrometer/core/instrument/composite/CompositeCustomMeter.java Show resolved Hide resolved

shakuzen changed the title ~~Bugfix/issue 1159 (Removing registry from CompositeMeterRegistry)~~ Removing registry from CompositeMeterRegistry removes composite meters added to the removed registry

admoca60 commented

View reviewed changes

...r-core/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java Show resolved Hide resolved

...meter-core/src/main/java/io/micrometer/core/instrument/composite/AbstractCompositeMeter.java

                   public final void remove(MeterRegistry registry) {
                       for (; ; ) {
                           if (childrenGuard.compareAndSet(false, true)) {
                               try {
                                   Map<MeterRegistry, T> newChildren = new IdentityHashMap<>(children);
-                                  newChildren.remove(registry);
+                                  newChildren.computeIfPresent(registry, (reg, met) -> {

Author

admoca60 Nov 27, 2020

@shakuzen taking into consideration your PR #2362, this change is not required. We can rollback the changes in this file.

micrometer-core/src/main/java/io/micrometer/core/instrument/composite/CompositeCustomMeter.java Show resolved Hide resolved

...meter-core/src/main/java/io/micrometer/core/instrument/composite/CompositeMeterRegistry.java Show resolved Hide resolved

...r-core/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java Show resolved Hide resolved

...r-core/src/test/java/io/micrometer/core/instrument/composite/CompositeMeterRegistryTest.java Show resolved Hide resolved

pivotal-issuemaster commented Mar 22, 2021

@admoca60 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

5 similar comments

pivotal-issuemaster commented Mar 22, 2021

@admoca60 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster commented Mar 22, 2021

@admoca60 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster commented Mar 22, 2021

@admoca60 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster commented Mar 22, 2021

@admoca60 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster commented Mar 22, 2021

@admoca60 Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

Author

admoca60 commented Mar 22, 2021

I had a mistake and I answered your comments at last Nov 20th, but I forgot click on "Submit Review". Currently, most of these comments are obsoleted, but one of them was very important. I will copy&paste it to review. I was asking about the expected behaviour of a metric after the child registry was removed and added (again) to a composite meter registry.

The comment was:

In fact, I think that this test is not really good because is testing a very strange use case. Anyway, we should define what will be the final approach in case that the child registry goes again to be added to composite.
do we want that the meters will be recreated? the meters will be relinked to composite?

In my opinion, if a child registry is removed, all referenced meters should be removed and then, in case of someone wants to add again the child, a new collection of linked meters will be created and set to the same values that parent.

Do you agree?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment