[GitHub] [incubator-openwhisk] style95 commented on a change in pull request #4561: Add more diagnostic information to completion ack processing

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

style95 commented on a change in pull request #4561: Add more diagnostic information to completion ack processing
URL: https://github.com/apache/incubator-openwhisk/pull/4561#discussion_r305568689
 
 

 ##########
 File path: common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala
 ##########
 @@ -414,6 +414,28 @@ object LoggingMarkers {
       LogMarkerToken(loadbalancer + controllerInstance.asString, s"memory${actionType}Inflight", counter)(
         MeasurementUnit.none)
 
+  // Counter metrics for completion acks in load balancer
+  sealed trait completionAckType { def asString: String = ??? }
+  case object regularCompletionAck extends completionAckType { override def asString = "regular" }
+  case object forcedCompletionAck extends completionAckType { override def asString = "forced" }
+  case object healthcheckCompletionAck extends completionAckType { override def asString = "healthcheck" }
+  case object regularAfterForcedCompletionAck extends completionAckType { override def asString = "regularAfterForced" }
+  case object forcedAfterRegularCompletionAck extends completionAckType { override def asString = "forcedAfterRegular" }
 
 Review comment:
   This change would be useful to diagnose operational issues.
   I have a few comments.
   
   In a small scope, if you don't have any other purpose to use `toString`, I think you can directly override `toString` here.
   Then you can just use that like this: `"type" -> completionAckType`.
   
   And I prefer to use the upper case first character for the object and trait according to the scala convention.
   https://docs.scala-lang.org/style/naming-conventions.html
   
   
   In a bigger scope, this is a good way to define subclass(object) but if this trait is only used here I'd rather define them using just `Sting`.
   
   ```scala
   def LOADBALANCER_COMPLETION_ACK(controllerInstance: ControllerInstanceId, completionAckType: String) =
       if (TransactionId.metricsKamonTags)
         LogMarkerToken(
           loadbalancer,
           "completionAck",
           counter,
           None,
           Map("controller_id" -> controllerInstance.asString, "type" -> completionAckType))(MeasurementUnit.none)
       else
         LogMarkerToken(
           loadbalancer + controllerInstance.asString,
           "completionAck_" + completionAckType,
           counter)(MeasurementUnit.none)
   ```
   
   If you mind a client uses this with just string, I think you can also define them as a `Constant`.
   
   ```scala
   val RegularCompletionAck: String = "regular"
   .
   .
   MetricEmitter.emitCounterMetric(
               LoggingMarkers.LOADBALANCER_COMPLETION_ACK(controllerInstance, RegularCompletionAck))
           }
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to 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