From a13915d973c9583d93670f6559e89c7f1fbe2651 Mon Sep 17 00:00:00 2001 From: HYEONSEOK1 <37043329+HYEONSEOK1@users.noreply.github.com> Date: Thu, 1 Aug 2024 07:22:15 +0900 Subject: [PATCH] Add default factoryType tag in CommonsObjectPool2Metrics (#5316) There are cases where the factoryType tag is missing which leads to issues in certain registries (i.e.: Prometheus). This change adds a default "none" value to the factoryType tag so that it will be always present. Closes gh-5316 --------- Co-authored-by: Jonatan Ivanov --- .../CommonsObjectPool2Metrics.java | 65 ++++++++++--------- .../CommonsObjectPool2MetricsTest.java | 50 +++++++------- 2 files changed, 61 insertions(+), 54 deletions(-) diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/commonspool2/CommonsObjectPool2Metrics.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/commonspool2/CommonsObjectPool2Metrics.java index 0eb5a16b17..acc67005af 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/commonspool2/CommonsObjectPool2Metrics.java +++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/binder/commonspool2/CommonsObjectPool2Metrics.java @@ -129,13 +129,18 @@ public void bindTo(@NonNull MeterRegistry registry) { private Iterable nameTag(ObjectName name, String type) throws AttributeNotFoundException, MBeanException, ReflectionException, InstanceNotFoundException { - Tags tags = Tags.of("name", name.getKeyProperty("name"), "type", type); + return Tags.of("name", name.getKeyProperty("name"), "type", type, "factoryType", getFactoryType(name, type)); + } + + private String getFactoryType(ObjectName name, String type) + throws ReflectionException, AttributeNotFoundException, InstanceNotFoundException, MBeanException { if (Objects.equals(type, "GenericObjectPool")) { // for GenericObjectPool, we want to include the name and factoryType as tags - String factoryType = mBeanServer.getAttribute(name, "FactoryType").toString(); - tags = Tags.concat(tags, "factoryType", factoryType); + return mBeanServer.getAttribute(name, "FactoryType").toString(); + } + else { + return "none"; } - return tags; } private void registerMetricsEventually(String type, BiConsumer perObject) { @@ -170,36 +175,34 @@ private void registerNotificationListener(String type, BiConsumer { - executor.execute(() -> { - MBeanServerNotification mbs = (MBeanServerNotification) notification; - ObjectName o = mbs.getMBeanName(); - Iterable nameTags = emptyList(); - int maxTries = 3; - for (int i = 0; i < maxTries; i++) { - try { - Thread.sleep(1000); - } - catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new RuntimeException(e); - } - try { - nameTags = nameTag(o, type); - break; - } - catch (AttributeNotFoundException | MBeanException | ReflectionException - | InstanceNotFoundException e) { - if (i == maxTries - 1) { - log.error("can not set name tag", e); - } + (notification, handback) -> executor.execute(() -> { + MBeanServerNotification mbs = (MBeanServerNotification) notification; + ObjectName o = mbs.getMBeanName(); + Iterable nameTags = emptyList(); + int maxTries = 3; + for (int i = 0; i < maxTries; i++) { + try { + Thread.sleep(1000); + } + catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } + try { + nameTags = nameTag(o, type); + break; + } + catch (AttributeNotFoundException | MBeanException | ReflectionException + | InstanceNotFoundException e) { + if (i == maxTries - 1) { + log.error("can not set name tag", e); } } - perObject.accept(o, Tags.concat(tags, nameTags)); - }); - }; + } + perObject.accept(o, Tags.concat(tags, nameTags)); + }); - NotificationFilter filter = (NotificationFilter) notification -> { + NotificationFilter filter = notification -> { if (!MBeanServerNotification.REGISTRATION_NOTIFICATION.equals(notification.getType())) return false; ObjectName obj = ((MBeanServerNotification) notification).getMBeanName(); diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/commonspool2/CommonsObjectPool2MetricsTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/commonspool2/CommonsObjectPool2MetricsTest.java index 5f8bb62284..e975cb508e 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/commonspool2/CommonsObjectPool2MetricsTest.java +++ b/micrometer-core/src/test/java/io/micrometer/core/instrument/binder/commonspool2/CommonsObjectPool2MetricsTest.java @@ -26,6 +26,7 @@ import org.apache.commons.pool2.impl.GenericObjectPool; import org.apache.commons.pool2.impl.GenericObjectPoolConfig; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import java.util.Arrays; @@ -41,44 +42,50 @@ class CommonsObjectPool2MetricsTest { private int genericObjectPoolCount = 0; - private Tags tags = Tags.of("app", "myapp", "version", "1"); + private final Tags tags = Tags.of("app", "myapp", "version", "1"); - private final MeterRegistry registry = new SimpleMeterRegistry(); + private MeterRegistry registry; private final CommonsObjectPool2Metrics commonsObjectPool2Metrics = new CommonsObjectPool2Metrics(tags); + @BeforeEach + void setUp() { + registry = new SimpleMeterRegistry(); + } + @AfterEach - void afterEach() { + void tearDown() { commonsObjectPool2Metrics.close(); } @Test - void verifyMetricsWithExpectedTags() { - try (GenericObjectPool p = createGenericObjectPool()) { - MeterRegistry registry = new SimpleMeterRegistry(); + void verifyGenericObjectPoolMetricsWithExpectedTags() { + try (GenericObjectPool ignored = createGenericObjectPool()) { commonsObjectPool2Metrics.bindTo(registry); + Tags tagsToMatch = tags.and("name", "pool", "type", "GenericObjectPool", "factoryType", + "io.micrometer.core.instrument.binder.commonspool2.CommonsObjectPool2MetricsTest$1"); - registry.get("commons.pool2.num.idle").tags(tags).gauge(); - registry.get("commons.pool2.num.waiters").tags(tags).gauge(); + registry.get("commons.pool2.num.idle").tags(tagsToMatch).gauge(); + registry.get("commons.pool2.num.waiters").tags(tagsToMatch).gauge(); Arrays .asList("commons.pool2.created", "commons.pool2.borrowed", "commons.pool2.returned", "commons.pool2.destroyed", "commons.pool2.destroyed.by.evictor", "commons.pool2.destroyed.by.borrow.validation") - .forEach(name -> registry.get(name).tags(tags).functionCounter()); + .forEach(name -> registry.get(name).tags(tagsToMatch).functionCounter()); Arrays .asList("commons.pool2.max.borrow.wait", "commons.pool2.mean.active", "commons.pool2.mean.idle", "commons.pool2.mean.borrow.wait") - .forEach(name -> registry.get(name).tags(tags).timeGauge()); + .forEach(name -> registry.get(name).tags(tagsToMatch).timeGauge()); } } @Test void verifyGenericKeyedObjectPoolMetricsWithExpectedTags() { - try (GenericKeyedObjectPool p = createGenericKeyedObjectPool()) { - Tags tagsToMatch = tags.and("type", "GenericKeyedObjectPool"); + try (GenericKeyedObjectPool ignored = createGenericKeyedObjectPool()) { commonsObjectPool2Metrics.bindTo(registry); + Tags tagsToMatch = tags.and("name", "pool", "type", "GenericKeyedObjectPool", "factoryType", "none"); Arrays.asList("commons.pool2.num.idle", "commons.pool2.num.waiters") .forEach(name -> registry.get(name).tags(tagsToMatch).gauge()); @@ -106,13 +113,13 @@ void verifyIdleAndActiveInstancesAreReported() throws Exception { }); try (GenericObjectPool genericObjectPool = createGenericObjectPool()) { - latch.await(10, TimeUnit.SECONDS); - final Object o = genericObjectPool.borrowObject(10_000L); + assertThat(latch.await(10, TimeUnit.SECONDS)).isTrue(); + Object object = genericObjectPool.borrowObject(10_000L); assertThat(registry.get("commons.pool2.num.active").gauge().value()).isEqualTo(1.0); assertThat(registry.get("commons.pool2.num.idle").gauge().value()).isEqualTo(0.0); - genericObjectPool.returnObject(o); + genericObjectPool.returnObject(object); assertThat(registry.get("commons.pool2.num.active").gauge().value()).isEqualTo(0.0); assertThat(registry.get("commons.pool2.num.idle").gauge().value()).isEqualTo(1.0); @@ -121,11 +128,9 @@ void verifyIdleAndActiveInstancesAreReported() throws Exception { @Test void metricsReportedPerMultiplePools() { - try (final GenericObjectPool p1 = createGenericObjectPool(); - final GenericObjectPool p2 = createGenericObjectPool(); - final GenericObjectPool p3 = createGenericObjectPool()) { - - MeterRegistry registry = new SimpleMeterRegistry(); + try (GenericObjectPool ignored1 = createGenericObjectPool(); + GenericObjectPool ignored2 = createGenericObjectPool(); + GenericObjectPool ignored3 = createGenericObjectPool()) { commonsObjectPool2Metrics.bindTo(registry); registry.get("commons.pool2.num.waiters").tag("name", "pool" + genericObjectPoolCount).gauge(); @@ -135,7 +140,6 @@ void metricsReportedPerMultiplePools() { @Test void newPoolsAreDiscoveredByListener() throws InterruptedException { - MeterRegistry registry = new SimpleMeterRegistry(); commonsObjectPool2Metrics.bindTo(registry); CountDownLatch latch = new CountDownLatch(1); @@ -144,8 +148,8 @@ void newPoolsAreDiscoveredByListener() throws InterruptedException { latch.countDown(); }); - try (GenericObjectPool p = createGenericObjectPool()) { - latch.await(10, TimeUnit.SECONDS); + try (GenericObjectPool ignored = createGenericObjectPool()) { + assertThat(latch.await(10, TimeUnit.SECONDS)).isTrue(); } }