[#1149] Use all tags when reporting metrics to PrometheusMeterRegistry.

Kai Hudalla · Kai Hudalla · commit 59da606a46f7 · 2019-04-09T14:46:47.000+02:00
The protocol adapters now always provide values for all tags when reporting telemetry messages. Fixes #1149 Signed-off-by: Kai Hudalla <kai.hudalla@bosch-si.com>
diff --git a/service-base/src/main/java/org/eclipse/hono/service/metric/MetricsTags.java b/service-base/src/main/java/org/eclipse/hono/service/metric/MetricsTags.java
@@ -196,7 +196,7 @@ public enum TtdStatus {
         /**
          * Status indicating that the message from the device did not contain a TTD value.
          */
-        NONE(),
+        NONE("none"),
         /**
          * Status indicating that the TTD expired without any pending commands for the device.
          */
@@ -226,24 +226,6 @@ public enum TtdStatus {
         public Tag asTag() {
             return tag;
         }
-
-        /**
-         * Adds a tag for the TTD status to a given set of tags.
-         * <p>
-         * The tag is only added if the status is not {@link #NONE}.
-         * 
-         * @param tags The tags to add to.
-         * @return The tags.
-         * @throws NullPointerException if tags is {@code null}.
-         */
-        public Tags add(final Tags tags) {
-            Objects.requireNonNull(tags);
-            if (tag == null) {
-                return tags;
-            } else {
-                return tags.and(tag);
-            }
-        }
     }
 
     /**
@@ -254,7 +236,7 @@ public enum QoS {
         /**
          * QoS indicating unknown delivery semantics.
          */
-        UNKNOWN(),
+        UNKNOWN("unknown"),
         /**
          * QoS (level 0) indicating at-most-once delivery semantics.
          */
@@ -301,24 +283,6 @@ public static QoS from(final int level) {
         public Tag asTag() {
             return tag;
         }
-
-        /**
-         * Adds a tag for the QoS level to a given set of tags.
-         * <p>
-         * The tag is only added if the status is not {@link #UNKNOWN}.
-         * 
-         * @param tags The tags to add to.
-         * @return The tags.
-         * @throws NullPointerException if tags is {@code null}.
-         */
-        public Tags add(final Tags tags) {
-            Objects.requireNonNull(tags);
-            if (tag == null) {
-                return tags;
-            } else {
-                return tags.and(tag);
-            }
-        }
     }
 
     /**
diff --git a/service-base/src/main/java/org/eclipse/hono/service/metric/MicrometerBasedMetrics.java b/service-base/src/main/java/org/eclipse/hono/service/metric/MicrometerBasedMetrics.java
@@ -172,17 +172,19 @@ public final void reportTelemetry(
             throw new IllegalArgumentException("payload size must not be negative");
         }
 
-        final Tags baseTags = Tags.of(type.asTag())
+        final Tags tags = Tags.of(type.asTag())
                 .and(MetricsTags.getTenantTag(tenantId))
-                .and(outcome.asTag());
+                .and(outcome.asTag())
+                .and(qos.asTag())
+                .and(ttdStatus.asTag());
 
-        timer.stop(this.registry.timer(METER_MESSAGES_RECEIVED, ttdStatus.add(qos.add(baseTags))));
+        timer.stop(this.registry.timer(METER_MESSAGES_RECEIVED, tags));
 
         // record payload size
         DistributionSummary.builder(METER_MESSAGES_PAYLOAD)
             .baseUnit("bytes")
             .minimumExpectedValue(0L)
-            .tags(baseTags)
+            .tags(tags)
             .register(this.registry)
             .record(payloadSize);
 
@@ -233,17 +235,17 @@ public void reportCommand(
             throw new IllegalArgumentException("payload size must not be negative");
         }
 
-        final Tags baseTags = Tags.of(direction.asTag())
+        final Tags tags = Tags.of(direction.asTag())
                 .and(MetricsTags.getTenantTag(tenantId))
                 .and(outcome.asTag());
 
-        timer.stop(this.registry.timer(METER_COMMANDS_RECEIVED, baseTags));
+        timer.stop(this.registry.timer(METER_COMMANDS_RECEIVED, tags));
 
         // record payload size
         DistributionSummary.builder(METER_COMMANDS_PAYLOAD)
             .baseUnit("bytes")
             .minimumExpectedValue(0L)
-            .tags(baseTags)
+            .tags(tags)
             .register(this.registry)
             .record(payloadSize);
 
@@ -284,7 +286,6 @@ protected <K, V extends Number> V gaugeForKey(final String name, final Map<K, V>
             return this.registry.gauge(name, tags, instanceSupplier.get());
 
         });
-
     }
 
     /**
@@ -304,6 +305,5 @@ protected <V extends Number> V gaugeForTenant(final String name, final Map<Strin
             final Supplier<V> instanceSupplier) {
 
         return gaugeForKey(name, map, tenant, Tags.of(MetricsTags.getTenantTag(tenant)), instanceSupplier);
-
     }
 }
diff --git a/service-base/src/test/java/org/eclipse/hono/service/metric/MicrometerBasedMetricsTest.java b/service-base/src/test/java/org/eclipse/hono/service/metric/MicrometerBasedMetricsTest.java
@@ -15,44 +15,116 @@
 package org.eclipse.hono.service.metric;
 
 import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.mockito.Mockito.*;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+
+import java.util.Arrays;
+import java.util.Collection;
 
 import org.eclipse.hono.service.metric.MetricsTags.EndpointType;
 import org.eclipse.hono.service.metric.MetricsTags.QoS;
 import org.eclipse.hono.service.metric.MetricsTags.TtdStatus;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+import org.junit.runners.Parameterized.Parameters;
 
+import io.micrometer.core.instrument.Clock;
 import io.micrometer.core.instrument.MeterRegistry;
 import io.micrometer.core.instrument.Tags;
-import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
+import io.micrometer.core.instrument.Timer.Sample;
+import io.micrometer.graphite.GraphiteConfig;
+import io.micrometer.graphite.GraphiteMeterRegistry;
+import io.micrometer.prometheus.PrometheusConfig;
+import io.micrometer.prometheus.PrometheusMeterRegistry;
 
 
 /**
  * Verifies behavior of {@link MicrometerBasedMetrics}.
  *
  */
+@RunWith(Parameterized.class)
 public class MicrometerBasedMetricsTest {
 
-    private MeterRegistry registry;
+    /**
+     * The Micrometer registry to run the tests against.
+     */
+    @Parameter
+    public MeterRegistry registry;
     private MicrometerBasedMetrics metrics;
 
+    /**
+     * Gets the Micrometer registries that the tests should be run against.
+     * 
+     * @return The registries.
+     */
+    @Parameters(name = "{0}")
+    public static Collection<MeterRegistry> registries() {
+        return Arrays.asList(new MeterRegistry[] {
+                                new PrometheusMeterRegistry(PrometheusConfig.DEFAULT),
+                                new GraphiteMeterRegistry(GraphiteConfig.DEFAULT, Clock.SYSTEM)
+                                });
+    }
+
     /**
      * Sets up the fixture.
      */
     @Before
     public void setUp() {
-        registry = new SimpleMeterRegistry();
         metrics = new MicrometerBasedMetrics(registry);
     }
 
+    /**
+     * Verifies that arbitrary telemetry messages with or without a QoS
+     * can be reported successfully.
+     */
+    @Test
+    public void testReportTelemetryWithOptionalQos() {
+
+        // GIVEN a sample
+        final Sample sample = metrics.startTimer();
+
+        // WHEN reporting a telemetry message with a QoS of AT_LEAST_ONCE
+        // and no TTD
+        metrics.reportTelemetry(
+                MetricsTags.EndpointType.TELEMETRY,
+                "tenant",
+                MetricsTags.ProcessingOutcome.FORWARDED,
+                MetricsTags.QoS.AT_LEAST_ONCE,
+                1024,
+                MetricsTags.TtdStatus.NONE,
+                sample);
+
+        // THEN the meter can be found in the registry with the tags that have a known value
+        final Tags expectedTags = Tags.of(MetricsTags.EndpointType.TELEMETRY.asTag())
+                .and(MetricsTags.getTenantTag("tenant"))
+                .and(MetricsTags.ProcessingOutcome.FORWARDED.asTag())
+                .and(MetricsTags.QoS.AT_LEAST_ONCE.asTag());
+        assertNotNull(registry.find(MicrometerBasedMetrics.METER_MESSAGES_RECEIVED).tags(expectedTags).timer());
+
+        // and reporting another telemetry message with no QoS but with a TTD status succeeds
+
+        final Sample otherSample = metrics.startTimer();
+
+        metrics.reportTelemetry(
+                MetricsTags.EndpointType.TELEMETRY,
+                "tenant",
+                MetricsTags.ProcessingOutcome.FORWARDED,
+                MetricsTags.QoS.UNKNOWN,
+                1024,
+                MetricsTags.TtdStatus.EXPIRED,
+                otherSample);
+    }
+
     /**
      * Verifies that when reporting a downstream message no tags for
      * {@link QoS#UNKNOWN} nor {@link TtdStatus#NONE} are included.
      */
     @Test
-    public void testReportTelemetryOmitsOptionalTags() {
+    public void testReportTelemetryWithUnknownTagValues() {
 
         metrics.reportTelemetry(
                 MetricsTags.EndpointType.TELEMETRY,
@@ -65,14 +137,11 @@ public void testReportTelemetryOmitsOptionalTags() {
 
         final Tags expectedTags = Tags.of(MetricsTags.EndpointType.TELEMETRY.asTag())
                 .and(MetricsTags.getTenantTag("tenant"))
-                .and(MetricsTags.ProcessingOutcome.FORWARDED.asTag());
+                .and(MetricsTags.ProcessingOutcome.FORWARDED.asTag())
+                .and(MetricsTags.QoS.UNKNOWN.asTag())
+                .and(MetricsTags.TtdStatus.NONE.asTag());
 
         assertNotNull(registry.find(MicrometerBasedMetrics.METER_MESSAGES_RECEIVED).tags(expectedTags).timer());
-
-        assertNull(registry.find(MicrometerBasedMetrics.METER_MESSAGES_RECEIVED)
-                .tags(expectedTags).tagKeys(MetricsTags.QoS.TAG_NAME).timer());
-        assertNull(registry.find(MicrometerBasedMetrics.METER_MESSAGES_RECEIVED)
-                .tags(expectedTags).tagKeys(MetricsTags.TtdStatus.TAG_NAME).timer());
     }
 
     /**
diff --git a/site/content/api/Metrics.md b/site/content/api/Metrics.md
@@ -58,10 +58,10 @@ Additional tags for protocol adapters are:
 | Name        | Value                                              | Description |
 | ----------- | -------------------------------------------------- | ----------- |
 | *direction* | `one-way`, `request`, `response`               | The direction in which a Command &amp; Control message is being sent:<br>`one-way` indicates a command sent to a device for which the sending application doesn't expect to receive a response.<br>`request` indicates a command request message sent to a device.<br>`response` indicates a command response received from a device. |
-| *qos*       | `0`, `1`                                          | The quality of service used for a telemetry or event message.<br>`0` indicates *at most once*,<br>`1` indicates *at least once* delivery semantics.<br>This tag will be omitted if the quality of service cannot be determined. |
+| *qos*       | `0`, `1`, `unknown`                              | The quality of service used for a telemetry or event message.<br>`0` indicates *at most once*,<br>`1` indicates *at least once* and<br> `none` indicates unknown delivery semantics. |
 | *status*    | `forwarded`, `unprocessable`, `undeliverable` | The processing status of a message.<br>`forwarded` indicates that the message has been forwarded to a downstream consumer<br>`unprocessable` indicates that the message has not been processed not forwarded, e.g. because the message was malformed<br>`undeliverable` indicates that the message could not be forwarded, e.g. because there is no downstream consumer or due to an infrastructure problem |
 | *tenant*    | *string*                                           | The identifier of the tenant that the metric is being reported for |
-| *ttd*       | `command`, `expired`                             | A status indicating the outcome of processing a TTD value contained in a message received from a device.<br>`command` indicates that a command for the device has been included in the response to the device's request for uploading the message.<br>`expired` indicates that a response without a command has been sent to the device<br>Note that this tag is only used by protocol adapters which use a request/response based transport protocol like HTTP. The tag will be omitted if the device did not specify a TTD value in its message. |
+| *ttd*       | `command`, `expired`, `none`                    | A status indicating the outcome of processing a TTD value contained in a message received from a device.<br>`command` indicates that a command for the device has been included in the response to the device's request for uploading the message.<br>`expired` indicates that a response without a command has been sent to the device.<br>`none` indicates that either no TTD value has been specified by the device or that the protocol adapter does not support it. |
 | *type*      | `telemetry`, `event`                             | The type of (downstream) message that the metric is being reported for. |
 
 Metrics provided by the protocol adapters are:
@@ -72,11 +72,9 @@ Metrics provided by the protocol adapters are:
 | *hono.commands.payload*            | DistributionSummary | *host*, *component-type*, *component-name*, *tenant*, *type*, *status*, *direction*          | The number of bytes conveyed in the payload of a command message. |
 | *hono.connections.authenticated*   | Gauge               | *host*, *component-type*, *component-name*, *tenant*                                         | Current number of connected, authenticated devices. <br/> **NB** This metric is only supported by protocol adapters that maintain *connection state* with authenticated devices. In particular, the HTTP adapter does not support this metric. |
 | *hono.connections.unauthenticated* | Gauge               | *host*, *component-type*, *component-name*                                                   | Current number of connected, unauthenticated devices. <br/> **NB** This metric is only supported by protocol adapters that maintain *connection state* with authenticated devices. In particular, the HTTP adapter does not support this metric. |
-| *hono.messages.received*           | Timer               | *host*, *component-type*, *component-name*, *tenant*, *type*, *status*, \[*qos*,\] \[*ttd*\] | The time it took to process a message conveying telemetry data or an event. |
+| *hono.messages.received*           | Timer               | *host*, *component-type*, *component-name*, *tenant*, *type*, *status*, *qos*, *ttd*         | The time it took to process a message conveying telemetry data or an event. |
 | *hono.messages.payload*            | DistributionSummary | *host*, *component-type*, *component-name*, *tenant*, *type*, *status*                       | The number of bytes conveyed in the payload of a telemetry or event message. |
 
-A tag name in square brackets indicates that the tag may not be used with each reported value.
-
 ### Service Metrics
 
 Hono's service components do not report any metrics at the moment.
diff --git a/site/content/release-notes.md b/site/content/release-notes.md
@@ -18,6 +18,10 @@ title = "Release Notes"
   processing of requests at a high level.
 * The `hono-client` and `hono-core` artifacts use Java 8 level again so that they
   can be used in applications using Java 8.
+* The protocol adapters now always specify values for the *ttd* and *qos* tags when
+  reporting telemetry messages using meter name *hono.messages.received*. This fixes
+  an issue when using the Prometheus back end where the HTTP adapter failed to report
+  messages that contained a TTD value and others that didn't.
 
 ### API Changes
 
