Merge pull request #299 from cliveseldon/custom_metric_fixes

Fix storing of Gauge metrics
SeldonIO · Nov 15, 2018 · b406c23 · b406c23
2 parents 38f22e5 + 9363930
commit b406c23
Show file tree

Hide file tree

Showing 5 changed files with 136 additions and 15 deletions.
diff --git a/engine/src/main/java/io/seldon/engine/metrics/CustomMetricsManager.java b/engine/src/main/java/io/seldon/engine/metrics/CustomMetricsManager.java
@@ -2,12 +2,15 @@
 
 import java.util.concurrent.ConcurrentHashMap;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 import org.springframework.stereotype.Component;
 
 import com.google.common.util.concurrent.AtomicDouble;
 
+import io.micrometer.core.instrument.Meter;
 import io.micrometer.core.instrument.Metrics;
-import io.seldon.engine.predictors.PredictiveUnitState;
+import io.micrometer.core.instrument.Tag;
 import io.seldon.protos.PredictionProtos.Metric;
 
 /**
@@ -19,17 +22,21 @@
 @Component
 public class CustomMetricsManager {
 
-	private ConcurrentHashMap<PredictiveUnitState,AtomicDouble> gauges = new ConcurrentHashMap<>();
+	private final static Logger logger = LoggerFactory.getLogger(CustomMetricsManager.class);
+	private ConcurrentHashMap<Meter.Id,AtomicDouble> gauges = new ConcurrentHashMap<>();
 
-	public AtomicDouble get(PredictiveUnitState state,Metric metric)
+
+	public AtomicDouble get(Iterable<Tag> tags,Metric metric)
 	{
-		if (gauges.containsKey(state))
-			return gauges.get(state);
+		Meter.Id id = Metrics.globalRegistry.createId(metric.getKey(), tags, "");
+		if (gauges.containsKey(id))
+			return gauges.get(id);
 		else
 		{
+			logger.info("Creating new metric Id for {}",metric.toString());
 			AtomicDouble d = new AtomicDouble();
-			gauges.put(state, d);
-			Metrics.globalRegistry.gauge(metric.getKey(), d);
+			gauges.put(id, d);
+			Metrics.globalRegistry.gauge(metric.getKey(), tags, d);
 			return d;
 		}
 	}

diff --git a/engine/src/main/java/io/seldon/engine/predictors/PredictiveUnitBean.java b/engine/src/main/java/io/seldon/engine/predictors/PredictiveUnitBean.java
@@ -287,21 +287,21 @@ protected void doStoreFeedbackMetrics(Feedback feedback, PredictiveUnitState sta
 
 	private void addCustomMetrics(List<Metric> metrics, PredictiveUnitState state)
 	{
-		logger.debug("Add metrics");
+		logger.info("Add metrics");
 		for(Metric metric : metrics)
 		{
 			switch(metric.getType())
 			{
 			case COUNTER:
-				logger.debug("Adding counter {} for {}",metric.getKey(),state.name);
+				logger.info("Adding counter {} for {}",metric.getKey(),state.name);
 				Counter.builder(metric.getKey()).tags(tagsProvider.getModelMetrics(state)).register(Metrics.globalRegistry).increment(metric.getValue());
 				break;
 			case GAUGE:
-				logger.debug("Adding gauge {} for {}",metric.getKey(),state.name);				
-				customMetricsManager.get(state, metric).set(metric.getValue());
+				logger.info("Adding gauge {} for {}",metric.getKey(),state.name);				
+				customMetricsManager.get(tagsProvider.getModelMetrics(state), metric).set(metric.getValue());
 				break;
 			case TIMER:
-				logger.debug("Adding timer {} for {}",metric.getKey(),state.name);				
+				logger.info("Adding timer {} for {}",metric.getKey(),state.name);				
 				Timer.builder(metric.getKey()).tags(tagsProvider.getModelMetrics(state)).register(Metrics.globalRegistry).record((long) metric.getValue(), TimeUnit.MILLISECONDS);
 				break;
 			case UNRECOGNIZED:

diff --git a/engine/src/test/java/io/seldon/engine/api/rest/TestRestClientControllerExternalGraphs.java b/engine/src/test/java/io/seldon/engine/api/rest/TestRestClientControllerExternalGraphs.java
@@ -11,10 +11,11 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-import org.mockito.InjectMocks;
 import org.mockito.Matchers;
 import org.mockito.Mock;
 import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.context.embedded.LocalServerPort;
 import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
@@ -110,6 +111,7 @@ public void testModelMetrics() throws Exception
     {
     	String jsonStr = readFile("src/test/resources/model_simple.json",StandardCharsets.UTF_8);
     	String responseStr = readFile("src/test/resources/response_with_metrics.json",StandardCharsets.UTF_8);
+    	String responseStr2 = readFile("src/test/resources/response_with_metrics2.json",StandardCharsets.UTF_8);
     	PredictorSpec.Builder PredictorSpecBuilder = PredictorSpec.newBuilder();
     	updateMessageBuilderFromJson(PredictorSpecBuilder, jsonStr);
     	PredictorSpec predictorSpec = PredictorSpecBuilder.build();
@@ -120,9 +122,19 @@ public void testModelMetrics() throws Exception
     	enginePredictor.setPredictorSpec(predictorSpec);
 
 
-    	ResponseEntity<String> httpResponse = new ResponseEntity<String>(responseStr, null, HttpStatus.OK);
+    	ResponseEntity<String> httpResponse1 = new ResponseEntity<String>(responseStr, null, HttpStatus.OK);
+    	ResponseEntity<String> httpResponse2 = new ResponseEntity<String>(responseStr2, null, HttpStatus.OK);
     	Mockito.when(restTemplate.postForEntity(Matchers.<URI>any(), Matchers.<HttpEntity<MultiValueMap<String, String>>>any(), Matchers.<Class<String>>any()))
-    		.thenReturn(httpResponse);
+    		.thenAnswer(new Answer<ResponseEntity<String>>() {
+    		    private int count = 0;
+
+    		    public ResponseEntity<String> answer(InvocationOnMock invocation) {
+    		    	count++;
+    		        if (count == 1)
+    		            return httpResponse1;
+
+    		        return httpResponse2;
+    		    }});
     	internalPredictionService.setRestTemplate(restTemplate);
 
     	MvcResult res = mvc.perform(MockMvcRequestBuilders.post("/api/v0.1/predictions")
@@ -156,6 +168,41 @@ public void testModelMetrics() throws Exception
 	    response = res2.getResponse().getContentAsString();
 	    Assert.assertTrue(response.indexOf("mycounter_total{deployment_name=\"None\",model_image=\"seldonio/mean_classifier\",model_name=\"mean-classifier\",model_version=\"0.6\",predictor_name=\"fx-market-predictor\",predictor_version=\"unknown\",} 1.0")>-1);
 	    Assert.assertTrue(response.indexOf("mytimer_duration_seconds_count{deployment_name=\"None\",model_image=\"seldonio/mean_classifier\",model_name=\"mean-classifier\",model_version=\"0.6\",predictor_name=\"fx-market-predictor\",predictor_version=\"unknown\",} 1.0")>-1);
+	    Assert.assertTrue(response.indexOf("mygauge{deployment_name=\"None\",model_image=\"seldonio/mean_classifier\",model_name=\"mean-classifier\",model_version=\"0.6\",predictor_name=\"fx-market-predictor\",predictor_version=\"unknown\",} 22.0")>-1);	    
+    	System.out.println(response);
+
+    	res = mvc.perform(MockMvcRequestBuilders.post("/api/v0.1/predictions")
+    			.accept(MediaType.APPLICATION_JSON_UTF8)
+    			.content(predictJson)
+    			.contentType(MediaType.APPLICATION_JSON_UTF8)).andReturn();
+    	response = res.getResponse().getContentAsString();
+    	System.out.println(response);
+    	Assert.assertEquals(200, res.getResponse().getStatus());
+
+    	builder = SeldonMessage.newBuilder();
+	    JsonFormat.parser().ignoringUnknownFields().merge(response, builder);
+	    seldonMessage = builder.build();
+
+    	 // Check for returned metrics
+	    Assert.assertEquals("COUNTER",seldonMessage.getMeta().getMetrics(0).getType().toString());
+	    Assert.assertEquals(1.0F,seldonMessage.getMeta().getMetrics(0).getValue(),0.0);
+	    Assert.assertEquals("mycounter",seldonMessage.getMeta().getMetrics(0).getKey());
+
+	    Assert.assertEquals("GAUGE",seldonMessage.getMeta().getMetrics(1).getType().toString());
+	    Assert.assertEquals(100.0F,seldonMessage.getMeta().getMetrics(1).getValue(),0.0);
+	    Assert.assertEquals("mygauge",seldonMessage.getMeta().getMetrics(1).getKey());
+
+	    Assert.assertEquals("TIMER",seldonMessage.getMeta().getMetrics(2).getType().toString());
+	    Assert.assertEquals(1.0F,seldonMessage.getMeta().getMetrics(2).getValue(),0.0);
+	    Assert.assertEquals("mytimer",seldonMessage.getMeta().getMetrics(2).getKey());
+
+	 // Check prometheus endpoint for metric
+	    res2 = mvc.perform(MockMvcRequestBuilders.get("/prometheus")).andReturn();
+	    Assert.assertEquals(200, res2.getResponse().getStatus());
+	    response = res2.getResponse().getContentAsString();
+	    Assert.assertTrue(response.indexOf("mycounter_total{deployment_name=\"None\",model_image=\"seldonio/mean_classifier\",model_name=\"mean-classifier\",model_version=\"0.6\",predictor_name=\"fx-market-predictor\",predictor_version=\"unknown\",} 2.0")>-1);
+	    Assert.assertTrue(response.indexOf("mytimer_duration_seconds_count{deployment_name=\"None\",model_image=\"seldonio/mean_classifier\",model_name=\"mean-classifier\",model_version=\"0.6\",predictor_name=\"fx-market-predictor\",predictor_version=\"unknown\",} 2.0")>-1);
+	    Assert.assertTrue(response.indexOf("mygauge{deployment_name=\"None\",model_image=\"seldonio/mean_classifier\",model_name=\"mean-classifier\",model_version=\"0.6\",predictor_name=\"fx-market-predictor\",predictor_version=\"unknown\",} 100.0")>-1);
     	System.out.println(response);
     }
 

diff --git a/engine/src/test/java/io/seldon/engine/grpc/MetricsTest.java b/engine/src/test/java/io/seldon/engine/grpc/MetricsTest.java
@@ -0,0 +1,38 @@
+package io.seldon.engine.grpc;
+
+import java.util.Arrays;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import com.google.common.util.concurrent.AtomicDouble;
+
+import io.micrometer.core.instrument.Tag;
+import io.seldon.engine.metrics.CustomMetricsManager;
+import io.seldon.protos.PredictionProtos.Metric;
+import io.seldon.protos.PredictionProtos.Metric.MetricType;
+
+
+public class MetricsTest {
+
+	@Test
+	public void testGauge()
+	{
+		CustomMetricsManager m = new CustomMetricsManager();
+		Iterable<Tag> tags = Arrays.asList(Tag.of("tag1", "tag1value1"));
+		Metric metric1 = Metric.newBuilder().setKey("key1").setValue(1.0f).setType(MetricType.GAUGE).build();
+		AtomicDouble v1 = m.get(tags, metric1);
+		v1.set(metric1.getValue());
+		Metric metric2 = Metric.newBuilder().setKey("key1").setValue(2.0f).setType(MetricType.GAUGE).build();
+		AtomicDouble v2 = m.get(tags, metric2);
+		Assert.assertEquals(1.0D, v2.get(),0.01);
+		Assert.assertEquals(v1, v2);
+		Metric metric3 = Metric.newBuilder().setKey("key2").setValue(2.0f).setType(MetricType.GAUGE).build();
+		AtomicDouble v3 = m.get(tags, metric3);
+		Assert.assertNotEquals(v1, v3);
+		v2.set(metric2.getValue());
+		AtomicDouble v4 = m.get(tags, metric1);
+		Assert.assertEquals(2.0D, v4.get(),0.01);
+	}
+
+}
diff --git a/engine/src/test/resources/response_with_metrics2.json b/engine/src/test/resources/response_with_metrics2.json
@@ -0,0 +1,29 @@
+{
+	"meta": {
+		"metrics": [
+			{
+				"type": "COUNTER",
+				"key": "mycounter",
+				"value": 1.0
+			},
+			{
+				"type": "GAUGE",
+				"key": "mygauge",
+				"value": 100.0
+			},
+			{
+				"type": "TIMER",
+				"key": "mytimer",
+				"value": 1.0
+			}
+		]
+	},
+	"data": {
+		"ndarray": [
+			[
+				1,
+				2
+			]
+		]
+	}
+}