Always use ThingUid as the unique identifier for thing status metrics (…

…#3674) * Always use ThingUid as the unique identifier for thing status metrics At bind time, ThingId was being used, but then ThingUid was being used when changes occured, causing multiple meters to be created for the same Thing, with the status always being 0 for the meter created at bind time. This change uses the full ThingUid, for both binding and event processing, since ThingUid is the globally unique value. Fixes #3672 Signed-off-by: Scott Hraban <[email protected]>
openhab · Jul 2, 2023 · 505e512 · 505e512
1 parent db97610
commit 505e512
Show file tree

Hide file tree

Showing 2 changed files with 104 additions and 8 deletions.
diff --git a/....monitor/src/main/java/org/openhab/core/io/monitor/internal/metrics/ThingStateMetric.java b/....monitor/src/main/java/org/openhab/core/io/monitor/internal/metrics/ThingStateMetric.java
@@ -40,16 +40,18 @@
 import io.micrometer.core.instrument.Tags;
 
 /**
- * The {@link ThingStateMetric} class implements a metric for the openHAB things states.
+ * The {@link ThingStateMetric} class implements a metric for the openHAB things
+ * states.
  *
  * @author Robert Bach - Initial contribution
+ * @author Scott Hraban - Create Meter using thingUid instead of thingId during
+ *         bind phase
  */
 @NonNullByDefault
 public class ThingStateMetric implements OpenhabCoreMeterBinder, EventSubscriber {
     private final Logger logger = LoggerFactory.getLogger(ThingStateMetric.class);
     public static final String METRIC_NAME = "openhab.thing.state";
     private static final String THING_TAG_NAME = "thing";
-    private static final String THINGSTATUS_TOPIC_PREFIX = "openhab/things/";
     private final ThingRegistry thingRegistry;
     private final Meter.Id commonMeterId;
     private final Map<Meter.Id, AtomicInteger> registeredMeters = new HashMap<>();
@@ -70,7 +72,7 @@ public void bindTo(@NonNullByDefault({}) MeterRegistry meterRegistry) {
         logger.debug("ThingStateMetric is being bound...");
         this.meterRegistry = meterRegistry;
         thingRegistry.getAll().forEach(
-                thing -> createOrUpdateMetricForBundleState(thing.getUID().getId(), thing.getStatus().ordinal()));
+                thing -> createOrUpdateMetricForBundleState(thing.getUID().getAsString(), thing.getStatus().ordinal()));
         eventSubscriberRegistration = this.bundleContext.registerService(EventSubscriber.class.getName(), this, null);
     }
 
@@ -111,10 +113,13 @@ public Set<String> getSubscribedEventTypes() {
 
     @Override
     public void receive(Event event) {
-        logger.trace("Received ThingStatusInfo(Changed)Event...");
-        String thingId = event.getTopic().substring(THINGSTATUS_TOPIC_PREFIX.length(),
-                event.getTopic().lastIndexOf('/'));
-        ThingStatus status = gson.fromJson(event.getPayload(), ThingStatusInfo.class).getStatus();
-        createOrUpdateMetricForBundleState(thingId, status.ordinal());
+        if (event instanceof ThingStatusInfoEvent thingEvent) {
+            logger.trace("Received ThingStatusInfo(Changed)Event...");
+            String thingUid = thingEvent.getThingUID().getAsString();
+            ThingStatus status = gson.fromJson(event.getPayload(), ThingStatusInfo.class).getStatus();
+            createOrUpdateMetricForBundleState(thingUid, status.ordinal());
+        } else {
+            logger.trace("Received unsubscribed for event type {}", event.getClass().getSimpleName());
+        }
     }
 }
diff --git a/...itor/src/test/java/org/openhab/core/io/monitor/internal/metrics/ThingStateMetricTest.java b/...itor/src/test/java/org/openhab/core/io/monitor/internal/metrics/ThingStateMetricTest.java
@@ -0,0 +1,91 @@
+/**
+ * Copyright (c) 2010-2023 Contributors to the openHAB project
+ *
+ * See the NOTICE file(s) distributed with this work for additional
+ * information.
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ */
+package org.openhab.core.io.monitor.internal.metrics;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.mock;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+
+import org.eclipse.jdt.annotation.NonNullByDefault;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.junit.jupiter.MockitoExtension;
+import org.openhab.core.thing.Thing;
+import org.openhab.core.thing.ThingRegistry;
+import org.openhab.core.thing.ThingStatus;
+import org.openhab.core.thing.ThingStatusDetail;
+import org.openhab.core.thing.ThingStatusInfo;
+import org.openhab.core.thing.ThingTypeUID;
+import org.openhab.core.thing.ThingUID;
+import org.openhab.core.thing.events.ThingEventFactory;
+import org.openhab.core.thing.internal.ThingImpl;
+import org.osgi.framework.BundleContext;
+
+import io.micrometer.core.instrument.Meter;
+import io.micrometer.core.instrument.Tag;
+import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
+
+/**
+ * Tests for ThingStateMetric class
+ *
+ * @author Scott Hraban - Initial contribution
+ */
+@ExtendWith(MockitoExtension.class)
+@NonNullByDefault
+public class ThingStateMetricTest {
+
+    @Test
+    public void testThingUidAlwaysUsedToCreateMeter() {
+        final String strThingTypeUid = "sonos:Amp";
+
+        final String strThingUid = strThingTypeUid + ":RINCON_347E5C0D150501400";
+        ThingUID thingUid = new ThingUID(strThingUid);
+        Thing thing = new ThingImpl(new ThingTypeUID(strThingTypeUid), thingUid);
+
+        final String strThingUid2 = strThingTypeUid + ":foo";
+        ThingUID thingUid2 = new ThingUID(strThingUid2);
+
+        ThingRegistry thingRegistry = mock(ThingRegistry.class);
+
+        SimpleMeterRegistry meterRegistry = new SimpleMeterRegistry();
+
+        ThingStateMetric thingStateMetric = new ThingStateMetric(mock(BundleContext.class), thingRegistry,
+                new HashSet<Tag>());
+
+        // Only one meter registered at bind time
+        doReturn(Collections.singleton(thing)).when(thingRegistry).getAll();
+        thingStateMetric.bindTo(meterRegistry);
+
+        List<Meter> meters = meterRegistry.getMeters();
+        assertEquals(1, meters.size());
+        assertEquals(strThingUid, meters.get(0).getId().getTag("thing"));
+
+        // Still only one meter registered after receiving an event
+        ThingStatusInfo thingStatusInfo = new ThingStatusInfo(ThingStatus.ONLINE, ThingStatusDetail.NONE, null);
+        thingStateMetric.receive(ThingEventFactory.createStatusInfoEvent(thingUid, thingStatusInfo));
+
+        meters = meterRegistry.getMeters();
+        assertEquals(1, meters.size());
+        assertEquals(strThingUid, meters.get(0).getId().getTag("thing"));
+
+        // Now another one is added
+        thingStateMetric.receive(ThingEventFactory.createStatusInfoEvent(thingUid2, thingStatusInfo));
+
+        meters = meterRegistry.getMeters();
+        assertEquals(2, meters.size());
+    }
+}