Do not remap id in remove method (#2025)

Registering a meter to a `MeterRegistry` returns a meter with a mapped id (`MeterFilter`s `map` method applied to it), so we should allow users to pass that to the remove method and it work. The previous behavior mapped the ID (again), which was problematic if any `MeterFilter` mapping was not idempotent. Fixes #1850
micrometer-metrics · Apr 24, 2020 · 9b2c805 · 9b2c805
1 parent 685c5d2
commit 9b2c805
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 7 deletions.
diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/MeterRegistry.java
@@ -573,10 +573,10 @@ private Meter getOrCreateMeter(@Nullable DistributionStatisticConfig config,
 
                     m = builder.apply(mappedId, config);
 
-                    Id synAssoc = originalId.syntheticAssociation();
+                    Id synAssoc = mappedId.syntheticAssociation();
                     if (synAssoc != null) {
                         PSet<Id> existingSynthetics = syntheticAssociations.getOrDefault(synAssoc, HashTreePSet.empty());
-                        syntheticAssociations = syntheticAssociations.plus(synAssoc, existingSynthetics.plus(originalId));
+                        syntheticAssociations = syntheticAssociations.plus(synAssoc, existingSynthetics.plus(mappedId));
                     }
 
                     for (Consumer<Meter> onAdd : meterAddedListeners) {
@@ -614,14 +614,13 @@ public Meter remove(Meter meter) {
     }
 
     /**
-     * @param id The id of the meter to remove
+     * @param mappedId The id of the meter to remove
      * @return The removed meter, or null if no meter matched the provided id.
      * @since 1.1.0
      */
     @Incubating(since = "1.1.0")
     @Nullable
-    public Meter remove(Meter.Id id) {
-        Id mappedId = getMappedId(id);
+    public Meter remove(Meter.Id mappedId) {
         Meter m = meterMap.get(mappedId);
 
         if (m != null) {
@@ -630,10 +629,10 @@ public Meter remove(Meter.Id id) {
                 if (m != null) {
                     meterMap = meterMap.minus(mappedId);
 
-                    for (Id synthetic : syntheticAssociations.getOrDefault(id, HashTreePSet.empty())) {
+                    for (Id synthetic : syntheticAssociations.getOrDefault(mappedId, HashTreePSet.empty())) {
                         remove(synthetic);
                     }
-                    syntheticAssociations = syntheticAssociations.minus(id);
+                    syntheticAssociations = syntheticAssociations.minus(mappedId);
 
                     for (Consumer<Meter> onRemove : meterRemovedListeners) {
                         onRemove.accept(m);

diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/MeterRegistryTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/MeterRegistryTest.java
@@ -131,6 +131,21 @@ public Meter.Id map(Meter.Id id) {
         assertThat(registry.find("another.name").counter()).isNull();
     }
 
+    @Test
+    void removeMetersAffectedByNonIdempotentMeterFilter() {
+        registry.config().meterFilter(new MeterFilter() {
+            @Override
+            public Meter.Id map(Meter.Id id) {
+                return id.withName("prefix." + id.getName());
+            }
+        });
+
+        Counter counter = registry.counter("name");
+        assertThat(registry.find("prefix.name").counter()).isSameAs(counter);
+        assertThat(registry.remove(counter)).isSameAs(counter);
+        assertThat(registry.find("prefix.name").counter()).isNull();
+    }
+
     @Test
     void removeMetersWithSynthetics() {
         Timer timer = Timer.builder("my.timer")