Closes #587 - Using InspectITContext during metric recording and igno…

…ring isTag (#594)

inspectit-ocelot-agent/src/system-test/resources/config/UserInstrumentationWithMetricsTest.yml

@@ -36,11 +36,11 @@ inspectit:
         scopes:
           UserInstrumentationWithMetricsTest-invocationCount : true
         entry:
-          method_name:  {action: get_method_fqn}
+          current_method_name:  {action: get_method_fqn}
         metrics:
           '[my/invocation]':
             value: 42
-            data-tags: {method_name: method_name, user_tag: user_tag}
+            data-tags: {method_name: current_method_name, user_tag: user_tag}
 
       record_method_duration:
         scopes:

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/context/InspectitContextImpl.java

@@ -347,29 +347,22 @@ private boolean isInDifferentThreadThanParentOrIsParentClosed() {
     public Scope enterFullTagScope() {
         TagContextBuilder builder = Tags.getTagger().emptyBuilder();
         dataTagsStream()
-                .forEach(e -> builder.put(TagKey.create(e.getKey()), TagValue.create(e.getValue().toString())));
+                .forEach(e -> builder.putLocal(TagKey.create(e.getKey()), TagValue.create(e.getValue().toString())));
         return builder.buildScoped();
     }
 
-    /**
-     * Returns map of tags currently present in {@link #getData()}.
-     *
-     * @return the tag stream
-     * @see #enterFullTagScope()
-     */
-    public Map<String, Object> getFullTagMap() {
-        return dataTagsStream().collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue));
-    }
-
     private Stream<Map.Entry<String, Object>> dataTagsStream() {
         return getDataAsStream()
                 .filter(e -> propagation.isTag(e.getKey()))
                 .filter(e -> ALLOWED_TAG_TYPES.contains(e.getValue().getClass()));
     }
 
     /**
+     * Returns the most recent value for data, which either was inherited form the parent context,
+     * set via {@link #setData(String, Object)} or changed due to an up-propagation.
+     *
      * @param key the name of the data to query
-     * @return the most recent value for data, which either was inherited form the parent context, set via {@link #setData(String, Object)} or changed due to an up-propagation.
+     * @return the data element which is related to the given key or `null` if it doesn't exist
      */
     @Override
     public Object getData(String key) {

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHookGenerator.java

@@ -1,5 +1,6 @@
 package rocks.inspectit.ocelot.core.instrumentation.hook;
 
+import com.google.common.annotations.VisibleForTesting;
 import io.opencensus.stats.StatsRecorder;
 import io.opencensus.trace.Sampler;
 import io.opencensus.trace.samplers.Samplers;
@@ -173,27 +174,35 @@ private List<IHookAction> buildTracingExitActions(RuleTracingSettings tracing) {
     }
 
     private Optional<IHookAction> buildMetricsRecorder(MethodHookConfiguration config) {
-        Collection<MetricRecordingSettings> metrics = config.getMetrics();
-        if (!metrics.isEmpty()) {
-            List<MetricAccessor> metricAccessors = metrics.stream()
-                    .map(metricConfig -> {
-                        String value = metricConfig.getValue();
-                        VariableAccessor valueAccessor;
-                        try {
-                            valueAccessor = variableAccessorFactory.getConstantAccessor(Double.parseDouble(value));
-                        } catch (NumberFormatException e) {
-                            valueAccessor = variableAccessorFactory.getVariableAccessor(value);
-                        }
-                        return new MetricAccessor(metricConfig.getMetric(), valueAccessor, metricConfig.getConstantTags(), metricConfig.getDataTags());
-                    })
+        Collection<MetricRecordingSettings> metricRecordingSettings = config.getMetrics();
+        if (!metricRecordingSettings.isEmpty()) {
+            List<MetricAccessor> metricAccessors = metricRecordingSettings.stream()
+                    .map(this::buildMetricAccessor)
                     .collect(Collectors.toList());
+
             MetricsRecorder recorder = new MetricsRecorder(metricAccessors, commonTagsManager, metricsManager, statsRecorder);
             return Optional.of(recorder);
         } else {
             return Optional.empty();
         }
     }
 
+    @VisibleForTesting
+    MetricAccessor buildMetricAccessor(MetricRecordingSettings metricSettings) {
+        String value = metricSettings.getValue();
+        VariableAccessor valueAccessor;
+        try {
+            valueAccessor = variableAccessorFactory.getConstantAccessor(Double.parseDouble(value));
+        } catch (NumberFormatException e) {
+            valueAccessor = variableAccessorFactory.getVariableAccessor(value);
+        }
+
+        Map<String, VariableAccessor> tagAccessors = metricSettings.getDataTags().entrySet().stream()
+                .collect(Collectors.toMap(Map.Entry::getKey, entry -> variableAccessorFactory.getVariableAccessor(entry.getValue())));
+
+        return new MetricAccessor(metricSettings.getMetric(), valueAccessor, metricSettings.getConstantTags(), tagAccessors);
+    }
+
     private List<IHookAction> buildActionCalls(List<ActionCallConfig> calls, MethodReflectionInformation methodInfo) {
 
         List<IHookAction> result = new ArrayList<>();

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/MetricsRecorder.java

@@ -5,12 +5,12 @@
 import io.opencensus.tags.*;
 import lombok.Value;
 import lombok.extern.slf4j.Slf4j;
+import rocks.inspectit.ocelot.core.instrumentation.context.InspectitContextImpl;
 import rocks.inspectit.ocelot.core.instrumentation.hook.actions.model.MetricAccessor;
 import rocks.inspectit.ocelot.core.metrics.MeasuresAndViewsManager;
 import rocks.inspectit.ocelot.core.tags.CommonTagsManager;
 
 import java.util.List;
-import java.util.Map;
 import java.util.Optional;
 
 /**
@@ -42,50 +42,42 @@ public class MetricsRecorder implements IHookAction {
 
     @Override
     public void execute(ExecutionContext context) {
-        // get all available data from the context and collect in a map
-        Map<String, Object> contextTags = null;
-
         // then iterate all metrics and enter new scope for metric collection
         for (MetricAccessor metricAccessor : metrics) {
             Object value = metricAccessor.getVariableAccessor().get(context);
             if (value != null) {
                 if (value instanceof Number) {
-                    // resolve context tags on the first need for it
-                    if (contextTags == null) {
-                        contextTags = context.getInspectitContext().getFullTagMap();
-                    }
-
                     // only record metrics where a value is present
                     // this allows to disable the recording of a metric depending on the results of action executions
                     MeasureMap measureMap = statsRecorder.newMeasureMap();
                     metricsManager.tryRecordingMeasurement(metricAccessor.getName(), measureMap, (Number) value);
-                    TagContext tagContext = getTagContext(contextTags, metricAccessor);
+                    TagContext tagContext = getTagContext(context, metricAccessor);
                     measureMap.record(tagContext);
                 }
             }
         }
     }
 
-    private TagContext getTagContext(Map<String, Object> contextTags, MetricAccessor metricAccessor) {
+    private TagContext getTagContext(ExecutionContext context, MetricAccessor metricAccessor) {
+        InspectitContextImpl inspectitContext = context.getInspectitContext();
+
         // create builder
         TagContextBuilder builder = Tags.getTagger().emptyBuilder();
 
         // first common tags to allow overwrite by constant or data tags
         commonTagsManager.getCommonTagKeys()
-                .forEach(commonTagKey -> Optional.ofNullable(contextTags.get(commonTagKey.getName()))
-                                .ifPresent(value -> builder.putLocal(commonTagKey, TagValue.create(value.toString())))
-                        //TODO if not present in the context do we pull the value from the common tag map
+                .forEach(commonTagKey -> Optional.ofNullable(inspectitContext.getData(commonTagKey.getName()))
+                        .ifPresent(value -> builder.putLocal(commonTagKey, TagValue.create(value.toString())))
                 );
 
         // then constant tags to allow overwrite by data
         metricAccessor.getConstantTags()
                 .forEach((key, value) -> builder.putLocal(TagKey.create(key), TagValue.create(value)));
 
         // go over data tags and match the value to the key from the contextTags (if available)
-        metricAccessor.getDataTags()
-                .forEach((key, dataLink) -> Optional.ofNullable(contextTags.get(dataLink))
-                        .ifPresent(value -> builder.putLocal(TagKey.create(key), TagValue.create(value.toString())))
-                );
+        metricAccessor.getDataTagAccessors()
+                .forEach((key, accessor) -> Optional.ofNullable(accessor.get(context))
+                        .ifPresent(tagValue -> builder.putLocal(TagKey.create(key), TagValue.create(tagValue.toString()))));
 
         // build and return
         return builder.build();

inspectit-ocelot-core/src/main/java/rocks/inspectit/ocelot/core/instrumentation/hook/actions/model/MetricAccessor.java

@@ -30,8 +30,8 @@ public class MetricAccessor {
     private final Map<String, String> constantTags;
 
     /**
-     * Data tags keys and values.
+     * VariableAccessors for the data tags.
      */
-    private final Map<String, String> dataTags;
+    private final Map<String, VariableAccessor> dataTagAccessors;
 
 }

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/context/InspectitContextImplTest.java

@@ -850,12 +850,11 @@ void verifyLocalValuesPublishedCorrectly() {
                         .containsEntry("global", "globalValue");
             }
 
-            Map<String, Object> fullTagMap = ctx.getFullTagMap();
-            assertThat(fullTagMap).hasSize(2)
-                    .containsEntry("local", "localValue")
-                    .containsEntry("global", "globalValue");
+            assertThat(ctx.getData("local")).isEqualTo("localValue");
+            assertThat(ctx.getData("global")).isEqualTo("globalValue");
 
             ctx.close();
+
             assertThat(InspectitContextImpl.INSPECTIT_KEY.get()).isNull();
         }
 
@@ -893,13 +892,12 @@ void verifyUpPropagatedValuesOnlyAvailableInFullTagScope() {
                         .containsEntry("nestedKey2", "nestedValue2");
             }
 
-            Map<String, Object> fullTagMap = ctx.getFullTagMap();
-            assertThat(fullTagMap).hasSize(3)
-                    .containsEntry("rootKey1", "nestedValue1")
-                    .containsEntry("rootKey2", "rootValue2")
-                    .containsEntry("nestedKey2", "nestedValue2");
+            assertThat(ctx.getData("rootKey1")).isEqualTo("nestedValue1");
+            assertThat(ctx.getData("rootKey2")).isEqualTo("rootValue2");
+            assertThat(ctx.getData("nestedKey2")).isEqualTo("nestedValue2");
 
             ctx.close();
+
             assertThat(InspectitContextImpl.INSPECTIT_KEY.get()).isNull();
         }
 
@@ -931,12 +929,10 @@ void verifyOnlyPrimitiveDataUsedAsTag() {
                         .containsEntry("d4", "2.0");
             }
 
-            Map<String, Object> fullTagMap = ctx.getFullTagMap();
-            assertThat(fullTagMap).hasSize(4)
-                    .containsEntry("d1", "string")
-                    .containsEntry("d2", 1)
-                    .containsEntry("d3", 2L)
-                    .containsEntry("d4", 2.0d);
+            assertThat(ctx.getData("d1")).isEqualTo("string");
+            assertThat(ctx.getData("d2")).isEqualTo(1);
+            assertThat(ctx.getData("d3")).isEqualTo(2L);
+            assertThat(ctx.getData("d4")).isEqualTo(2.0D);
 
             ctx.close();
         }

inspectit-ocelot-core/src/test/java/rocks/inspectit/ocelot/core/instrumentation/hook/MethodHookGeneratorTest.java

@@ -9,11 +9,17 @@
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import rocks.inspectit.ocelot.config.model.instrumentation.rules.MetricRecordingSettings;
 import rocks.inspectit.ocelot.core.instrumentation.config.model.MethodHookConfiguration;
 import rocks.inspectit.ocelot.core.instrumentation.context.ContextManager;
+import rocks.inspectit.ocelot.core.instrumentation.hook.actions.model.MetricAccessor;
 import rocks.inspectit.ocelot.core.testutils.Dummy;
 
+import java.util.Collections;
+
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.entry;
+import static org.mockito.Mockito.*;
 
 @ExtendWith(MockitoExtension.class)
 public class MethodHookGeneratorTest {
@@ -24,6 +30,9 @@ public class MethodHookGeneratorTest {
     @InjectMocks
     MethodHookGenerator generator;
 
+    @Mock
+    VariableAccessorFactory variableAccessorFactory;
+
     @Nested
     class BuildHook {
 
@@ -79,6 +88,65 @@ void verifyDeclaringClassCorrect() {
 
             assertThat(result.getMethodInformation().getDeclaringClass()).isSameAs(Dummy.class);
         }
+    }
+
+    @Nested
+    class BuildMetricAccessor {
+
+        @Test
+        public void valueOnly() {
+            VariableAccessor mockAccessor = mock(VariableAccessor.class);
+            when(variableAccessorFactory.getConstantAccessor(1D)).thenReturn(mockAccessor);
+            MetricRecordingSettings settings = MetricRecordingSettings.builder().metric("name").value("1.0").build();
+
+            MetricAccessor accessor = generator.buildMetricAccessor(settings);
+
+            assertThat(accessor.getVariableAccessor()).isSameAs(mockAccessor);
+            assertThat(accessor.getConstantTags()).isEmpty();
+            assertThat(accessor.getDataTagAccessors()).isEmpty();
+        }
 
+        @Test
+        public void dataValueOnly() {
+            VariableAccessor mockAccessor = mock(VariableAccessor.class);
+            when(variableAccessorFactory.getVariableAccessor("data-key")).thenReturn(mockAccessor);
+            MetricRecordingSettings settings = MetricRecordingSettings.builder().metric("name").value("data-key").build();
+
+            MetricAccessor accessor = generator.buildMetricAccessor(settings);
+
+            assertThat(accessor.getVariableAccessor()).isSameAs(mockAccessor);
+            assertThat(accessor.getConstantTags()).isEmpty();
+            assertThat(accessor.getDataTagAccessors()).isEmpty();
+        }
+
+        @Test
+        public void valueAndConstantTag() {
+            VariableAccessor mockAccessor = mock(VariableAccessor.class);
+            when(variableAccessorFactory.getConstantAccessor(1D)).thenReturn(mockAccessor);
+            MetricRecordingSettings settings = MetricRecordingSettings.builder().metric("name").value("1.0")
+                    .constantTags(Collections.singletonMap("tag-key", "tag-key")).build();
+
+            MetricAccessor accessor = generator.buildMetricAccessor(settings);
+
+            assertThat(accessor.getVariableAccessor()).isSameAs(mockAccessor);
+            assertThat(accessor.getConstantTags()).containsOnly(entry("tag-key", "tag-key"));
+            assertThat(accessor.getDataTagAccessors()).isEmpty();
+        }
+
+        @Test
+        public void valueAndDataTag() {
+            VariableAccessor mockAccessorA = mock(VariableAccessor.class);
+            doReturn(mockAccessorA).when(variableAccessorFactory).getConstantAccessor(1D);
+            VariableAccessor mockAccessorB = mock(VariableAccessor.class);
+            doReturn(mockAccessorB).when(variableAccessorFactory).getVariableAccessor("tag-value");
+            MetricRecordingSettings settings = MetricRecordingSettings.builder().metric("name").value("1.0")
+                    .dataTags(Collections.singletonMap("tag-key", "tag-value")).build();
+
+            MetricAccessor accessor = generator.buildMetricAccessor(settings);
+
+            assertThat(accessor.getVariableAccessor()).isSameAs(mockAccessorA);
+            assertThat(accessor.getConstantTags()).isEmpty();
+            assertThat(accessor.getDataTagAccessors()).containsOnly(entry("tag-key", mockAccessorB));
+        }
     }
 }