[GitHub] chetanmeh commented on a change in pull request #4165: Update to Kamon 1.1.3 from 0.6 series

[GitBox <gi...@apache.org>]

chetanmeh commented on a change in pull request #4165: Update to Kamon 1.1.3 from 0.6 series
URL: https://github.com/apache/incubator-openwhisk/pull/4165#discussion_r245598370
 
 

 ##########
 File path: common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala
 ##########
 @@ -210,32 +210,43 @@ object LogMarkerToken {
 }
 
 object MetricEmitter {
-
-  val metrics = Kamon.metrics
+  import kamon.metric.InstrumentFactory.InstrumentTypes._
+  import kamon.metric.InstrumentFactory._
 
   def emitCounterMetric(token: LogMarkerToken, times: Long = 1): Unit = {
     if (TransactionId.metricsKamon) {
       if (TransactionId.metricsKamonTags) {
-        metrics
-          .counter(token.toString, token.tags)
+        Kamon
+          .counter(createName(token.toString, Counter))
+          .refine(token.tags)
           .increment(times)
       } else {
-        metrics.counter(token.toStringWithSubAction).increment(times)
+        Kamon.counter(createName(token.toStringWithSubAction, Counter)).increment(times)
       }
     }
   }
 
   def emitHistogramMetric(token: LogMarkerToken, value: Long): Unit = {
     if (TransactionId.metricsKamon) {
       if (TransactionId.metricsKamonTags) {
-        metrics
-          .histogram(token.toString, token.tags)
+        Kamon
+          .histogram(createName(token.toString, Histogram))
+          .refine(token.tags)
           .record(value)
       } else {
-        metrics.histogram(token.toStringWithSubAction).record(value)
+        Kamon.histogram(createName(token.toStringWithSubAction, Histogram)).record(value)
       }
     }
   }
+
+  /**
+   * Kamon 1.0 onwards does not include the metric type in the metric name which cause issue
+   * for us as we use same metric name for counter and histogram. So to be backward compatible we
+   * need to prefix the name with type
+   */
+  private def createName(name: String, metricType: InstrumentType) = {
+    s"${metricType.name.toLowerCase}.$name"
 
 Review comment:
   Currently the whole metric lookup flow gets invoked for each metric updated. To avoid churn it would be better to have direct reference to metric instance and then increment that. I was thinking to optimize this in a subsequent PR by storing metric instance in `LogMarkerToken` itself and avoid this whole logic on critical path

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services