Closes inspectIT#391 handle invalid metric tags

util method in CommonTagsManager to create TagValue for all invalid TagValues is <invalid> generated
arolfes · Apr 9, 2020 · f610417 · f610417
1 parent 1cde797
commit f610417
Show file tree

Hide file tree

Showing 10 changed files with 97 additions and 28 deletions.
diff --git a/...c/main/java/rocks/inspectit/ocelot/core/instrumentation/context/InspectitContextImpl.java b/...c/main/java/rocks/inspectit/ocelot/core/instrumentation/context/InspectitContextImpl.java
@@ -12,6 +12,7 @@
 import rocks.inspectit.ocelot.bootstrap.context.InternalInspectitContext;
 import rocks.inspectit.ocelot.config.model.instrumentation.data.PropagationMode;
 import rocks.inspectit.ocelot.core.instrumentation.config.model.propagation.PropagationMetaData;
+import rocks.inspectit.ocelot.core.tags.CommonTagsManager;
 
 import java.util.*;
 import java.util.stream.Stream;
@@ -346,7 +347,7 @@ private boolean isInDifferentThreadThanParentOrIsParentClosed() {
     public Scope enterFullTagScope() {
         TagContextBuilder builder = Tags.getTagger().emptyBuilder();
         dataTagsStream()
-                .forEach(e -> builder.putLocal(TagKey.create(e.getKey()), TagValue.create(e.getValue().toString())));
+                .forEach(e -> builder.putLocal(TagKey.create(e.getKey()), CommonTagsManager.createTagValue(e.getValue().toString())));
         return builder.buildScoped();
     }
 
@@ -579,7 +580,7 @@ private Iterator<Tag> getPostEntryPhaseTags() {
         return postEntryPhaseDownPropagatedData.entrySet().stream()
                 .filter(e -> propagation.isTag(e.getKey()))
                 .filter(e -> ALLOWED_TAG_TYPES.contains(e.getValue().getClass()))
-                .map(e -> Tag.create(TagKey.create(e.getKey()), TagValue.create(e.getValue().toString())))
+                .map(e -> Tag.create(TagKey.create(e.getKey()), CommonTagsManager.createTagValue(e.getValue().toString())))
                 .iterator();
     }
 

diff --git a/...c/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/MetricsRecorder.java b/...c/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/MetricsRecorder.java
@@ -2,7 +2,10 @@
 
 import io.opencensus.stats.MeasureMap;
 import io.opencensus.stats.StatsRecorder;
-import io.opencensus.tags.*;
+import io.opencensus.tags.TagContext;
+import io.opencensus.tags.TagContextBuilder;
+import io.opencensus.tags.TagKey;
+import io.opencensus.tags.Tags;
 import lombok.Value;
 import lombok.extern.slf4j.Slf4j;
 import rocks.inspectit.ocelot.core.instrumentation.context.InspectitContextImpl;
@@ -67,17 +70,17 @@ private TagContext getTagContext(ExecutionContext context, MetricAccessor metric
         // first common tags to allow overwrite by constant or data tags
         commonTagsManager.getCommonTagKeys()
                 .forEach(commonTagKey -> Optional.ofNullable(inspectitContext.getData(commonTagKey.getName()))
-                        .ifPresent(value -> builder.putLocal(commonTagKey, TagValue.create(value.toString())))
+                        .ifPresent(value -> builder.putLocal(commonTagKey, CommonTagsManager.createTagValue(value.toString())))
                 );
 
         // then constant tags to allow overwrite by data
         metricAccessor.getConstantTags()
-                .forEach((key, value) -> builder.putLocal(TagKey.create(key), TagValue.create(value)));
+                .forEach((key, value) -> builder.putLocal(TagKey.create(key), CommonTagsManager.createTagValue(value)));
 
         // go over data tags and match the value to the key from the contextTags (if available)
         metricAccessor.getDataTagAccessors()
                 .forEach((key, accessor) -> Optional.ofNullable(accessor.get(context))
-                        .ifPresent(tagValue -> builder.putLocal(TagKey.create(key), TagValue.create(tagValue.toString()))));
+                        .ifPresent(tagValue -> builder.putLocal(TagKey.create(key), CommonTagsManager.createTagValue(tagValue.toString()))));
 
         // build and return
         return builder.build();

diff --git a/...ocelot-core/src/main/java/rocks/inspectit/ocelot/core/metrics/jmx/JmxMetricsRecorder.java b/...ocelot-core/src/main/java/rocks/inspectit/ocelot/core/metrics/jmx/JmxMetricsRecorder.java
@@ -7,7 +7,6 @@
 import io.opencensus.stats.StatsRecorder;
 import io.opencensus.tags.TagContextBuilder;
 import io.opencensus.tags.TagKey;
-import io.opencensus.tags.TagValue;
 import io.opencensus.tags.Tagger;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -72,9 +71,9 @@ public JmxMetricsRecorder(Tagger tagger) {
     JmxMetricsRecorder(Tagger tagger, MeasuresAndViewsManager measuresAndViewsManager, StatsRecorder statsRecorder, CommonTagsManager commonTagsManager) {
         super("metrics.jmx");
         this.tagger = tagger;
-        this.measureManager = measuresAndViewsManager;
-        this.recorder = statsRecorder;
-        this.commonTags = commonTagsManager;
+        measureManager = measuresAndViewsManager;
+        recorder = statsRecorder;
+        commonTags = commonTagsManager;
     }
 
     /**
@@ -85,8 +84,8 @@ public JmxMetricsRecorder(Tagger tagger) {
     @Override
     protected boolean doEnable(InspectitConfig configuration) {
         // create a new scraper, called on every update of every jmx setting
-        this.jmxScraper = createScraper(configuration.getMetrics().getJmx(), this);
-        this.lowerCaseMetricName = configuration.getMetrics().getJmx().isLowerCaseMetricName();
+        jmxScraper = createScraper(configuration.getMetrics().getJmx(), this);
+        lowerCaseMetricName = configuration.getMetrics().getJmx().isLowerCaseMetricName();
 
         // call super to handle scheduling
         return super.doEnable(configuration);
@@ -137,7 +136,7 @@ public void recordBean(String domain, LinkedHashMap<String, String> beanProperti
             TagContextBuilder tagContextBuilder = tagger.currentBuilder();
             beanProperties.entrySet().stream()
                     .skip(1)
-                    .forEach(entry -> tagContextBuilder.putLocal(TagKey.create(entry.getKey()), TagValue.create(entry.getValue())));
+                    .forEach(entry -> tagContextBuilder.putLocal(TagKey.create(entry.getKey()), CommonTagsManager.createTagValue(entry.getValue())));
 
             MeasureMap measureMap = recorder.newMeasureMap();
             measureMap.put(measure, metricValue);

diff --git a/...elot-core/src/main/java/rocks/inspectit/ocelot/core/metrics/system/GCMetricsRecorder.java b/...elot-core/src/main/java/rocks/inspectit/ocelot/core/metrics/system/GCMetricsRecorder.java
@@ -4,7 +4,6 @@
 import com.sun.management.GcInfo;
 import io.opencensus.tags.TagContext;
 import io.opencensus.tags.TagKey;
-import io.opencensus.tags.TagValue;
 import io.opencensus.tags.Tagger;
 import lombok.extern.slf4j.Slf4j;
 import lombok.val;
@@ -14,6 +13,7 @@
 import rocks.inspectit.ocelot.config.model.metrics.MetricsSettings;
 import rocks.inspectit.ocelot.config.model.metrics.StandardMetricsSettings;
 import rocks.inspectit.ocelot.core.selfmonitoring.SelfMonitoringService;
+import rocks.inspectit.ocelot.core.tags.CommonTagsManager;
 
 import javax.management.ListenerNotFoundException;
 import javax.management.Notification;
@@ -203,8 +203,8 @@ private void recordConcurrentPhaseTime(GarbageCollectionNotificationInfo notific
         measureManager.getMeasureLong(CONCURRENT_PHASE_TIME_METRIC_FULL_NAME)
                 .ifPresent(measure -> {
                     TagContext tags = tagger.toBuilder(commonTags.getCommonTagContext())
-                            .putLocal(actionTagKey, TagValue.create(notificationInfo.getGcAction()))
-                            .putLocal(causeTagKey, TagValue.create(notificationInfo.getGcCause()))
+                            .putLocal(actionTagKey, CommonTagsManager.createTagValue(notificationInfo.getGcAction()))
+                            .putLocal(causeTagKey, CommonTagsManager.createTagValue(notificationInfo.getGcCause()))
                             .build();
 
                     recorder.newMeasureMap()
@@ -217,8 +217,8 @@ private void recordGCPause(GarbageCollectionNotificationInfo notificationInfo) {
         measureManager.getMeasureLong(PAUSE_METRIC_FULL_NAME)
                 .ifPresent(measure -> {
                     TagContext tags = tagger.toBuilder(commonTags.getCommonTagContext())
-                            .putLocal(actionTagKey, TagValue.create(notificationInfo.getGcAction()))
-                            .putLocal(causeTagKey, TagValue.create(notificationInfo.getGcCause()))
+                            .putLocal(actionTagKey, CommonTagsManager.createTagValue(notificationInfo.getGcAction()))
+                            .putLocal(causeTagKey, CommonTagsManager.createTagValue(notificationInfo.getGcCause()))
                             .build();
 
                     recorder.newMeasureMap()

diff --git a/...-core/src/main/java/rocks/inspectit/ocelot/core/metrics/system/MemoryMetricsRecorder.java b/...-core/src/main/java/rocks/inspectit/ocelot/core/metrics/system/MemoryMetricsRecorder.java
@@ -2,12 +2,12 @@
 
 import io.opencensus.tags.TagContext;
 import io.opencensus.tags.TagKey;
-import io.opencensus.tags.TagValue;
 import io.opencensus.tags.Tagger;
 import lombok.val;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Service;
 import rocks.inspectit.ocelot.config.model.metrics.MetricsSettings;
+import rocks.inspectit.ocelot.core.tags.CommonTagsManager;
 
 import java.lang.management.BufferPoolMXBean;
 import java.lang.management.ManagementFactory;
@@ -73,8 +73,8 @@ private void recordMemoryMetrics(Map<String, Boolean> enabledMetrics) {
             for (MemoryPoolMXBean memoryPoolBean : ManagementFactory.getPlatformMXBeans(MemoryPoolMXBean.class)) {
                 String area = MemoryType.HEAP.equals(memoryPoolBean.getType()) ? "heap" : "nonheap";
                 TagContext tags = tagger.currentBuilder()
-                        .putLocal(idTagKey, TagValue.create(memoryPoolBean.getName()))
-                        .putLocal(areaTagKey, TagValue.create(area))
+                        .putLocal(idTagKey, CommonTagsManager.createTagValue(memoryPoolBean.getName()))
+                        .putLocal(areaTagKey, CommonTagsManager.createTagValue(area))
                         .build();
 
                 val mm = recorder.newMeasureMap();
@@ -104,7 +104,7 @@ private void recordBufferMetrics(Map<String, Boolean> enabledMetrics) {
         if (bufferCountEnabled || bufferUsedEnabled || bufferCapacityEnabled) {
             for (BufferPoolMXBean bufferPoolBean : ManagementFactory.getPlatformMXBeans(BufferPoolMXBean.class)) {
                 TagContext tags = tagger.currentBuilder()
-                        .putLocal(idTagKey, TagValue.create(bufferPoolBean.getName()))
+                        .putLocal(idTagKey, CommonTagsManager.createTagValue(bufferPoolBean.getName()))
                         .build();
 
                 val mm = recorder.newMeasureMap();

diff --git a/...-core/src/main/java/rocks/inspectit/ocelot/core/metrics/system/ThreadMetricsRecorder.java b/...-core/src/main/java/rocks/inspectit/ocelot/core/metrics/system/ThreadMetricsRecorder.java
@@ -2,12 +2,12 @@
 
 import io.opencensus.tags.TagContextBuilder;
 import io.opencensus.tags.TagKey;
-import io.opencensus.tags.TagValue;
 import io.opencensus.tags.Tagger;
 import lombok.val;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Service;
 import rocks.inspectit.ocelot.config.model.metrics.MetricsSettings;
+import rocks.inspectit.ocelot.core.tags.CommonTagsManager;
 
 import java.lang.management.ManagementFactory;
 import java.lang.management.ThreadInfo;
@@ -80,7 +80,7 @@ private void recordStateMetric() {
         measureManager.getMeasureLong(METRIC_NAME_PREFIX + STATE_METRIC_NAME)
                 .ifPresent(measure -> {
                     for (val state : Thread.State.values()) {
-                        TagContextBuilder contextBuilder = tagger.currentBuilder().putLocal(stateTag, TagValue.create(state.name()));
+                        TagContextBuilder contextBuilder = tagger.currentBuilder().putLocal(stateTag, CommonTagsManager.createTagValue(state.name()));
                         try (val scope = contextBuilder.buildScoped()) {
                             val mm = recorder.newMeasureMap();
                             val count = Arrays.stream(threadBean.getThreadInfo(threadBean.getAllThreadIds()))

diff --git a/...-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/SelfMonitoringService.java b/...-core/src/main/java/rocks/inspectit/ocelot/core/selfmonitoring/SelfMonitoringService.java
@@ -3,7 +3,6 @@
 import io.opencensus.common.Scope;
 import io.opencensus.stats.StatsRecorder;
 import io.opencensus.tags.TagKey;
-import io.opencensus.tags.TagValue;
 import io.opencensus.tags.Tags;
 import lombok.Data;
 import lombok.extern.slf4j.Slf4j;
@@ -161,7 +160,7 @@ public void close() {
                     statsRecorder.newMeasureMap()
                             .put(m, durationInMicros)
                             .record(Tags.getTagger().toBuilder(commonTags.getCommonTagContext())
-                                    .putLocal(COMPONENT_TAG_KEY, TagValue.create(componentName)).build())
+                                    .putLocal(COMPONENT_TAG_KEY, CommonTagsManager.createTagValue(componentName)).build())
             );
 
             if (log.isTraceEnabled()) {

diff --git a/inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/tags/CommonTagsManager.java b/inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/tags/CommonTagsManager.java
@@ -3,6 +3,7 @@
 import io.opencensus.common.Scope;
 import io.opencensus.tags.*;
 import lombok.Getter;
+import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.context.event.EventListener;
 import org.springframework.core.Ordered;
@@ -20,6 +21,7 @@
  * Component that provides tags that should be considered as common and used when ever a metric is recorded.
  */
 @Component
+@Slf4j
 public class CommonTagsManager {
 
     /**
@@ -97,7 +99,7 @@ public Scope withCommonTagScope(Map<String, String> customTagMap) {
         HashMap<String, String> tags = new HashMap<>(commonTagValueMap);
         tags.putAll(customTagMap);
         tags.entrySet().stream()
-                .forEach(entry -> tagContextBuilder.putLocal(TagKey.create(entry.getKey()), TagValue.create(entry.getValue())));
+                .forEach(entry -> tagContextBuilder.putLocal(TagKey.create(entry.getKey()), createTagValue(entry.getValue())));
         return tagContextBuilder.buildScoped();
     }
 
@@ -123,11 +125,27 @@ private void update() {
         newCommonTagValueMap.forEach((k, v) -> {
             TagKey key = TagKey.create(k);
             newCommonTagKeys.add(key);
-            tagContextBuilder.putLocal(key, TagValue.create(v));
+            tagContextBuilder.putLocal(key, createTagValue(v));
         });
         commonTagKeys = newCommonTagKeys;
         commonTagValueMap = newCommonTagValueMap;
         commonTagContext = tagContextBuilder.build();
     }
 
+    /**
+     * Constructs a {@code io.opencensus.tags.TagValue} from the given string.
+     * If String is not valid it an <code>&lt;invalid&gt;</code> TagName is created.
+     *
+     * @param v the tag value
+     * @return the created TagValue with 'v' or '&lt;invalid&gt;'
+     */
+    public static TagValue createTagValue(String v) {
+        try {
+            return TagValue.create(v);
+        } catch (IllegalArgumentException e) {
+            log.warn("illegal tag value {} converted to <invalid>", v);
+            return TagValue.create("<invalid>");
+        }
+    }
+
 }
diff --git a/...-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/tags/CommonTagsManagerIntTest.java b/...-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/tags/CommonTagsManagerIntTest.java
@@ -99,4 +99,26 @@ public void extraOverwritesProviders() {
         }
     }
 
+    @Nested
+    @DirtiesContext
+    @TestPropertySource(properties = {
+            "inspectit.tags.extra.service-name=this-value-is-over-255-characters-long ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------"
+    })
+    class VeryLongTagValues extends SpringTestBase {
+
+        @Autowired
+        CommonTagsManager provider;
+
+        @Test
+        public void extraOverwritesProviders() {
+            TagContext commonTagContext = provider.getCommonTagContext();
+
+            assertThat(InternalUtils.getTags(commonTagContext))
+                    .anySatisfy(tag -> {
+                        assertThat(tag.getKey()).isEqualTo(TagKey.create("service-name"));
+                        assertThat(tag.getValue()).isEqualTo(TagValue.create("<invalid>"));
+                    });
+        }
+    }
+
 }
diff --git a/...tit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/tags/CommonTagsManagerTest.java b/...tit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/tags/CommonTagsManagerTest.java
@@ -0,0 +1,27 @@
+package rocks.inspectit.ocelot.core.tags;
+
+import io.opencensus.tags.TagValue;
+import org.junit.jupiter.api.Test;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class CommonTagsManagerTest {
+
+    @Test
+    public void createTagValue() {
+        assertThat(CommonTagsManager.createTagValue("my-tag-value")).isEqualTo(TagValue.create("my-tag-value"));
+    }
+
+    @Test
+    public void createTagValue_tooLong() {
+        assertThat(CommonTagsManager.createTagValue("this-value-is-over-255-characters-long ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------"))
+                .isEqualTo(TagValue.create("<invalid>"));
+    }
+
+    @Test
+    public void createTagValue_nonPrintableCharacter() {
+        assertThat(CommonTagsManager.createTagValue("non-printable-character-\u007f"))
+                .isEqualTo(TagValue.create("<invalid>"));
+    }
+
+}