You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2022/04/24 05:23:07 UTC

[GitHub] [openwhisk] ningyougang opened a new pull request, #5222: Add containerPool container histogram metric

ningyougang opened a new pull request, #5222:
URL: https://github.com/apache/openwhisk/pull/5222

   <!--- Provide a concise summary of your changes in the Title -->
   
   ## Description
   <!--- Provide a detailed description of your changes. -->
   <!--- Include details of what problem you are solving and how your changes are tested. -->
   
   ## Related issue and scope
   <!--- Please include a link to a related issue if there is one. -->
   - [ ] I opened an issue to propose and discuss this change (#????)
   
   ## My changes affect the following components
   <!--- Select below all system components are affected by your change. -->
   <!--- Enter an `x` in all applicable boxes. -->
   - [ ] API
   - [ ] Controller
   - [ ] Message Bus (e.g., Kafka)
   - [ ] Loadbalancer
   - [ ] Scheduler
   - [x] Invoker
   - [ ] Intrinsic actions (e.g., sequences, conductors)
   - [ ] Data stores (e.g., CouchDB)
   - [ ] Tests
   - [ ] Deployment
   - [ ] CLI
   - [ ] General tooling
   - [ ] Documentation
   
   ## Types of changes
   <!--- What types of changes does your code introduce? Use `x` in all the boxes that apply: -->
   - [ ] Bug fix (generally a non-breaking change which closes an issue).
   - [x] Enhancement or new feature (adds new functionality).
   - [ ] Breaking change (a bug fix or enhancement which changes existing behavior).
   
   ## Checklist:
   <!--- Please review the points below which help you make sure you've covered all aspects of the change you're making. -->
   
   - [x] I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
   - [x] I reviewed the [style guides](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md#coding-standards) and followed the recommendations (Travis CI will check :).
   - [ ] I added tests to cover my changes.
   - [ ] My changes require further changes to the documentation.
   - [ ] I updated the documentation where necessary.
   
   


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868827768


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala:
##########
@@ -176,6 +183,11 @@ class KubernetesClient(
         waitForPod(namespace, createdPod, start.start, config.timeouts.run)
           .map { readyPod =>
             transid.finished(this, start, logLevel = InfoLevel)
+            val t1 = System.currentTimeMillis()
+            MetricEmitter.emitHistogramMetric(
+              LogMarkerToken("kubeapi", "create", "duration", Some("create"), Map("cmd" -> "create"))(
+                MeasurementUnit.time.milliseconds),

Review Comment:
   Updated accordingly.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] bdoyle0182 commented on pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#issuecomment-1135076333

   @ningyougang can we get this merged in? This is a useful metric to have available along with the container pool memory that's already there


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] upgle commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
upgle commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868872324


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerClient.scala:
##########
@@ -204,7 +205,12 @@ class DockerClient(dockerHost: Option[String] = None,
       s"running ${cmd.mkString(" ")} (timeout: $timeout)",
       logLevel = InfoLevel)
     executeProcess(cmd, timeout).andThen {
-      case Success(_) => transid.finished(this, start)
+      case Success(_) =>
+        MetricEmitter.emitHistogramMetric(
+          LogMarkerToken("docker", "runCmd", "duration", Some(args.head), Map("cmd" -> args.head))(
+            MeasurementUnit.time.milliseconds),
+          Instant.now.toEpochMilli - transid.meta.start.toEpochMilli)

Review Comment:
   This is not the usual way to send metric.
   Is there any reason not to use transid start, finish ?



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang merged pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang merged PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r882354957


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala:
##########
@@ -110,7 +109,27 @@ class FunctionPullingContainerPool(
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("busy"), memoryConsumptionOf(busyPool))
     MetricEmitter
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("prewarmed"), memoryConsumptionOf(prewarmedPool))
+    MetricEmitter
+      .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("warmed"), memoryConsumptionOf(warmedPool))
     MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("max"), poolConfig.userMemory.toMB)
+    val prewarmedSize = prewarmedPool.size
+    val busySize = busyPool.size
+    val warmedSize = warmedPool.size
+    val warmedPoolGroupMap = warmedPool groupBy {
+      case (_, warmedData) => (warmedData.invocationNamespace, warmedData.action.toString)
+    }
+    val warmedPoolResultMap: mutable.Map[(String, String), Int] = mutable.Map.empty[(String, String), Int]
+    warmedPoolGroupMap.foreach { data =>
+      warmedPoolResultMap += (data._1 -> data._2.size)

Review Comment:
   There is an incompatibility problem in travis test.
   ![image](https://user-images.githubusercontent.com/11749867/170429818-451b6043-3bce-4538-9d42-bd2614e72b65.png)
   
   So don't use mapValues here due to scala2.13's return value is `MapView` and scala2.12's return type is `Map`



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r857084244


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala:
##########
@@ -111,6 +111,14 @@ class FunctionPullingContainerPool(
     MetricEmitter
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("prewarmed"), memoryConsumptionOf(prewarmedPool))
     MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("max"), poolConfig.userMemory.toMB)
+    val prewarmedSize = prewarmedPool.size
+    val busySize = busyPool.size
+    val warmedSize = warmedPool.size
+    val allSize = prewarmedSize + busySize + warmedSize
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("prewarmed"), prewarmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("busy"), busySize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("warmed"), warmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("all"), allSize)

Review Comment:
   As admin, he may want to know the prewarmed/busy/warmed container in one invoker in metric show page(e,g, grafana).



##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala:
##########
@@ -111,6 +111,14 @@ class FunctionPullingContainerPool(
     MetricEmitter
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("prewarmed"), memoryConsumptionOf(prewarmedPool))
     MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("max"), poolConfig.userMemory.toMB)
+    val prewarmedSize = prewarmedPool.size
+    val busySize = busyPool.size
+    val warmedSize = warmedPool.size
+    val allSize = prewarmedSize + busySize + warmedSize
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("prewarmed"), prewarmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("busy"), busySize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("warmed"), warmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("all"), allSize)

Review Comment:
   As admin, he may want to know the prewarmed/busy/warmed container in one invoker in metric show page



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] codecov-commenter commented on pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#issuecomment-1107787736

   # [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5222?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#5222](https://codecov.io/gh/apache/openwhisk/pull/5222?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (207b451) into [master](https://codecov.io/gh/apache/openwhisk/commit/3e3414ced7fb6e6d85b95f0f9cdd43a0a68da4fd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3e3414c) will **decrease** coverage by `39.94%`.
   > The diff coverage is `0.00%`.
   
   > :exclamation: Current head 207b451 differs from pull request most recent head 439d6d0. Consider uploading reports for the commit 439d6d0 to get more accurate results
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #5222       +/-   ##
   ==========================================
   - Coverage   44.49%   4.54%   -39.95%     
   ==========================================
     Files         238     238               
     Lines       13957   13972       +15     
     Branches      570     571        +1     
   ==========================================
   - Hits         6210     635     -5575     
   - Misses       7747   13337     +5590     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/openwhisk/pull/5222?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...in/scala/org/apache/openwhisk/common/Logging.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvbW1vbi9Mb2dnaW5nLnNjYWxh) | `41.17% <0.00%> (-33.83%)` | :arrow_down: |
   | [...re/containerpool/kubernetes/KubernetesClient.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC9rdWJlcm5ldGVzL0t1YmVybmV0ZXNDbGllbnQuc2NhbGE=) | `0.00% <0.00%> (ø)` | |
   | [...ontainerpool/v2/FunctionPullingContainerPool.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvY29udGFpbmVycG9vbC92Mi9GdW5jdGlvblB1bGxpbmdDb250YWluZXJQb29sLnNjYWxh) | `0.00% <0.00%> (-80.33%)` | :arrow_down: |
   | [.../main/scala/org/apache/openwhisk/core/WarmUp.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvV2FybVVwLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ain/scala/org/apache/openwhisk/spi/SpiLoader.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL3NwaS9TcGlMb2FkZXIuc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...scala/org/apache/openwhisk/core/FeatureFlags.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvRmVhdHVyZUZsYWdzLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...la/org/apache/openwhisk/http/BasicRasService.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2h0dHAvQmFzaWNSYXNTZXJ2aWNlLnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...pache/openwhisk/http/LenientSprayJsonSupport.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2h0dHAvTGVuaWVudFNwcmF5SnNvblN1cHBvcnQuc2NhbGE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...ache/openwhisk/core/database/DocumentFactory.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NjYWxhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvZGF0YWJhc2UvRG9jdW1lbnRGYWN0b3J5LnNjYWxh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...che/openwhisk/core/invoker/LogStoreCollector.scala](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29yZS9pbnZva2VyL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvb3BlbndoaXNrL2NvcmUvaW52b2tlci9Mb2dTdG9yZUNvbGxlY3Rvci5zY2FsYQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [121 more](https://codecov.io/gh/apache/openwhisk/pull/5222/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/openwhisk/pull/5222?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/openwhisk/pull/5222?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [3e3414c...439d6d0](https://codecov.io/gh/apache/openwhisk/pull/5222?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] jiangpengcheng commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r882409300


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala:
##########
@@ -110,7 +109,27 @@ class FunctionPullingContainerPool(
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("busy"), memoryConsumptionOf(busyPool))
     MetricEmitter
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("prewarmed"), memoryConsumptionOf(prewarmedPool))
+    MetricEmitter
+      .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("warmed"), memoryConsumptionOf(warmedPool))
     MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("max"), poolConfig.userMemory.toMB)
+    val prewarmedSize = prewarmedPool.size
+    val busySize = busyPool.size
+    val warmedSize = warmedPool.size
+    val warmedPoolGroupMap = warmedPool groupBy {
+      case (_, warmedData) => (warmedData.invocationNamespace, warmedData.action.toString)
+    }
+    val warmedPoolResultMap: mutable.Map[(String, String), Int] = mutable.Map.empty[(String, String), Int]
+    warmedPoolGroupMap.foreach { data =>
+      warmedPoolResultMap += (data._1 -> data._2.size)

Review Comment:
   can't it just remove the type annotation?
   
   ```scala
       val warmedPoolMap = warmedPool groupBy {
         case (_, warmedData) => (warmedData.invocationNamespace, warmedData.action.toString)
       } mapValues (_.size)
       for((data, size) <- warmedPoolMap) {
   ```



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868858429


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala:
##########
@@ -111,6 +111,14 @@ class FunctionPullingContainerPool(
     MetricEmitter
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("prewarmed"), memoryConsumptionOf(prewarmedPool))
     MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("max"), poolConfig.userMemory.toMB)
+    val prewarmedSize = prewarmedPool.size
+    val busySize = busyPool.size
+    val warmedSize = warmedPool.size
+    val allSize = prewarmedSize + busySize + warmedSize
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("prewarmed"), prewarmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("busy"), busySize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("warmed"), warmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("all"), allSize)

Review Comment:
   I guess that would apply for the memory metric as well



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868985343


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala:
##########
@@ -111,6 +111,14 @@ class FunctionPullingContainerPool(
     MetricEmitter
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("prewarmed"), memoryConsumptionOf(prewarmedPool))
     MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("max"), poolConfig.userMemory.toMB)
+    val prewarmedSize = prewarmedPool.size
+    val busySize = busyPool.size
+    val warmedSize = warmedPool.size
+    val allSize = prewarmedSize + busySize + warmedSize
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("prewarmed"), prewarmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("busy"), busySize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("warmed"), warmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("all"), allSize)

Review Comment:
   Regarding `I guess that would apply for the memory metric as well`
   Seems a little complex to apply tag: namespace + action to memory metric.
   Currently, i just apply the tag(namespace + action) to container metric.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868992430


##########
common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala:
##########
@@ -493,12 +502,10 @@ object LoggingMarkers {
   val INVOKER_ACTIVATION = LogMarkerToken(invoker, activation, start)(MeasurementUnit.none)
   def INVOKER_DOCKER_CMD(cmd: String) =
     LogMarkerToken(invoker, "docker", start, Some(cmd), Map("cmd" -> cmd))(MeasurementUnit.time.milliseconds)
-  def INVOKER_DOCKER_CMD_TIMEOUT(cmd: String) =
-    LogMarkerToken(invoker, "docker", timeout, Some(cmd), Map("cmd" -> cmd))(MeasurementUnit.none)
   def INVOKER_RUNC_CMD(cmd: String) =
     LogMarkerToken(invoker, "runc", start, Some(cmd), Map("cmd" -> cmd))(MeasurementUnit.time.milliseconds)
   def INVOKER_KUBEAPI_CMD(cmd: String) =
-    LogMarkerToken(invoker, "kubeapi", start, Some(cmd), Map("cmd" -> cmd))(MeasurementUnit.none)
+    LogMarkerToken(invoker, "kubeapi", start, Some(cmd), Map("cmd" -> cmd))(MeasurementUnit.time.milliseconds)

Review Comment:
   Obviously, the unit should be `MeasurementUnit.time.milliseconds`



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868993952


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerClient.scala:
##########
@@ -207,7 +207,6 @@ class DockerClient(dockerHost: Option[String] = None,
       case Success(_) => transid.finished(this, start)
       case Failure(pte: ProcessTimeoutException) =>
         transid.failed(this, start, pte.getMessage, ErrorLevel)
-        MetricEmitter.emitCounterMetric(LoggingMarkers.INVOKER_DOCKER_CMD_TIMEOUT(args.head))

Review Comment:
   `transid.failed` already send the metric, so delete the repeated one and delete `INVOKER_DOCKER_CMD_TIMEOUT` method



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] upgle commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
upgle commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868764475


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala:
##########
@@ -176,6 +183,11 @@ class KubernetesClient(
         waitForPod(namespace, createdPod, start.start, config.timeouts.run)
           .map { readyPod =>
             transid.finished(this, start, logLevel = InfoLevel)
+            val t1 = System.currentTimeMillis()
+            MetricEmitter.emitHistogramMetric(
+              LogMarkerToken("kubeapi", "create", "duration", Some("create"), Map("cmd" -> "create"))(
+                MeasurementUnit.time.milliseconds),

Review Comment:
   The related transaction already exists. And it is better to use transid than to calculate time directly.
   
   ```
       val start = transid.started(
         this,
         LoggingMarkers.INVOKER_KUBEAPI_CMD("create"),
         s"launching pod $name (image:$image, mem: ${memory.toMB}) (timeout: ${config.timeouts.run.toSeconds}s)",
         logLevel = akka.event.Logging.InfoLevel)
   ```



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r857084024


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerClient.scala:
##########
@@ -203,8 +203,15 @@ class DockerClient(dockerHost: Option[String] = None,
       LoggingMarkers.INVOKER_DOCKER_CMD(args.head),
       s"running ${cmd.mkString(" ")} (timeout: $timeout)",
       logLevel = InfoLevel)
+    val t0 = System.currentTimeMillis()
     executeProcess(cmd, timeout).andThen {
-      case Success(_) => transid.finished(this, start)
+      case Success(_) =>
+        val t1 = System.currentTimeMillis()
+        MetricEmitter.emitHistogramMetric(
+          LogMarkerToken("docker", "runCmd", "duration", Some(args.head), Map("cmd" -> args.head))(
+            MeasurementUnit.time.milliseconds),

Review Comment:
   In this pr, since `kubeapi for create pod consume time metric` is added, it is better to add `docker for create common container consume time metric`.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#issuecomment-1138040570

   > @ningyougang can we get this merged in? This is a useful metric to have available along with the container pool memory that's already there
   
   Oh, ok


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r857084024


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerClient.scala:
##########
@@ -203,8 +203,15 @@ class DockerClient(dockerHost: Option[String] = None,
       LoggingMarkers.INVOKER_DOCKER_CMD(args.head),
       s"running ${cmd.mkString(" ")} (timeout: $timeout)",
       logLevel = InfoLevel)
+    val t0 = System.currentTimeMillis()
     executeProcess(cmd, timeout).andThen {
-      case Success(_) => transid.finished(this, start)
+      case Success(_) =>
+        val t1 = System.currentTimeMillis()
+        MetricEmitter.emitHistogramMetric(
+          LogMarkerToken("docker", "runCmd", "duration", Some(args.head), Map("cmd" -> args.head))(
+            MeasurementUnit.time.milliseconds),

Review Comment:
   In this pr, since `kubeapi for create pod consume time metric` is added, it is better to add `docker for create common container consume time metric`.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r857083850


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala:
##########
@@ -176,6 +183,11 @@ class KubernetesClient(
         waitForPod(namespace, createdPod, start.start, config.timeouts.run)
           .map { readyPod =>
             transid.finished(this, start, logLevel = InfoLevel)
+            val t1 = System.currentTimeMillis()
+            MetricEmitter.emitHistogramMetric(
+              LogMarkerToken("kubeapi", "create", "duration", Some("create"), Map("cmd" -> "create"))(
+                MeasurementUnit.time.milliseconds),

Review Comment:
   Currently, just add kubeapi for create, maybe we can add kubeapi for delete in future.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] jiangpengcheng commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r882410364


##########
common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala:
##########
@@ -412,6 +412,15 @@ object LoggingMarkers {
     LogMarkerToken(invoker, "sharedPackage", counter, None, Map("path" -> path))(MeasurementUnit.none)
   def INVOKER_CONTAINERPOOL_MEMORY(state: String) =
     LogMarkerToken(invoker, "containerPoolMemory", counter, Some(state), Map("state" -> state))(MeasurementUnit.none)
+  def INVOKER_CONTAINERPOOL_CONTAINER(state: String, tags: Option[Map[String, String]] = None) = {
+    var map = Map("state" -> state)
+    tags.foreach { mapTags =>
+      for ((k, v) <- mapTags) {
+        map += (k -> v)
+      }
+    }
+    LogMarkerToken(invoker, "containerPoolContainer", counter, Some(state), map)(MeasurementUnit.none)

Review Comment:
   sorrry, this `Map("state" -> state) ++: tags.getOrElse(Map.empty)` should be better



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r882423765


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala:
##########
@@ -110,7 +109,27 @@ class FunctionPullingContainerPool(
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("busy"), memoryConsumptionOf(busyPool))
     MetricEmitter
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("prewarmed"), memoryConsumptionOf(prewarmedPool))
+    MetricEmitter
+      .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("warmed"), memoryConsumptionOf(warmedPool))
     MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("max"), poolConfig.userMemory.toMB)
+    val prewarmedSize = prewarmedPool.size
+    val busySize = busyPool.size
+    val warmedSize = warmedPool.size
+    val warmedPoolGroupMap = warmedPool groupBy {
+      case (_, warmedData) => (warmedData.invocationNamespace, warmedData.action.toString)
+    }
+    val warmedPoolResultMap: mutable.Map[(String, String), Int] = mutable.Map.empty[(String, String), Int]
+    warmedPoolGroupMap.foreach { data =>
+      warmedPoolResultMap += (data._1 -> data._2.size)

Review Comment:
   It is a good solution.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r882394436


##########
common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala:
##########
@@ -412,6 +412,15 @@ object LoggingMarkers {
     LogMarkerToken(invoker, "sharedPackage", counter, None, Map("path" -> path))(MeasurementUnit.none)
   def INVOKER_CONTAINERPOOL_MEMORY(state: String) =
     LogMarkerToken(invoker, "containerPoolMemory", counter, Some(state), Map("state" -> state))(MeasurementUnit.none)
+  def INVOKER_CONTAINERPOOL_CONTAINER(state: String, tags: Option[Map[String, String]] = None) = {
+    var map = Map("state" -> state)
+    tags.foreach { mapTags =>
+      for ((k, v) <- mapTags) {
+        map += (k -> v)
+      }
+    }
+    LogMarkerToken(invoker, "containerPoolContainer", counter, Some(state), map)(MeasurementUnit.none)

Review Comment:
   Updated accordingly.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang closed pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang closed pull request #5222: Add containerPool container histogram metric
URL: https://github.com/apache/openwhisk/pull/5222


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868930244


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala:
##########
@@ -111,6 +111,14 @@ class FunctionPullingContainerPool(
     MetricEmitter
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("prewarmed"), memoryConsumptionOf(prewarmedPool))
     MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("max"), poolConfig.userMemory.toMB)
+    val prewarmedSize = prewarmedPool.size
+    val busySize = busyPool.size
+    val warmedSize = warmedPool.size
+    val allSize = prewarmedSize + busySize + warmedSize
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("prewarmed"), prewarmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("busy"), busySize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("warmed"), warmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("all"), allSize)

Review Comment:
   @bdoyle0182 for warmed container size metric, i have added `namespace` and `action` tags.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868983272


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerClient.scala:
##########
@@ -204,7 +205,12 @@ class DockerClient(dockerHost: Option[String] = None,
       s"running ${cmd.mkString(" ")} (timeout: $timeout)",
       logLevel = InfoLevel)
     executeProcess(cmd, timeout).andThen {
-      case Success(_) => transid.finished(this, start)
+      case Success(_) =>
+        MetricEmitter.emitHistogramMetric(
+          LogMarkerToken("docker", "runCmd", "duration", Some(args.head), Map("cmd" -> args.head))(
+            MeasurementUnit.time.milliseconds),
+          Instant.now.toEpochMilli - transid.meta.start.toEpochMilli)

Review Comment:
   Already changed to use `transid.finish` to send the metric and delete the repeated one.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r857083850


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala:
##########
@@ -176,6 +183,11 @@ class KubernetesClient(
         waitForPod(namespace, createdPod, start.start, config.timeouts.run)
           .map { readyPod =>
             transid.finished(this, start, logLevel = InfoLevel)
+            val t1 = System.currentTimeMillis()
+            MetricEmitter.emitHistogramMetric(
+              LogMarkerToken("kubeapi", "create", "duration", Some("create"), Map("cmd" -> "create"))(
+                MeasurementUnit.time.milliseconds),

Review Comment:
   Currently, just add kubeapi for create, maybe we can add `kubeapi for delete` in future.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] upgle commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
upgle commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868764475


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala:
##########
@@ -176,6 +183,11 @@ class KubernetesClient(
         waitForPod(namespace, createdPod, start.start, config.timeouts.run)
           .map { readyPod =>
             transid.finished(this, start, logLevel = InfoLevel)
+            val t1 = System.currentTimeMillis()
+            MetricEmitter.emitHistogramMetric(
+              LogMarkerToken("kubeapi", "create", "duration", Some("create"), Map("cmd" -> "create"))(
+                MeasurementUnit.time.milliseconds),

Review Comment:
   The related transaction id already exists. And it is better to use transid than to calculate time directly.
   
   ```
       val start = transid.started(
         this,
         LoggingMarkers.INVOKER_KUBEAPI_CMD("create"),
         s"launching pod $name (image:$image, mem: ${memory.toMB}) (timeout: ${config.timeouts.run.toSeconds}s)",
         logLevel = akka.event.Logging.InfoLevel)
   ```



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868993952


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerClient.scala:
##########
@@ -207,7 +207,6 @@ class DockerClient(dockerHost: Option[String] = None,
       case Success(_) => transid.finished(this, start)
       case Failure(pte: ProcessTimeoutException) =>
         transid.failed(this, start, pte.getMessage, ErrorLevel)
-        MetricEmitter.emitCounterMetric(LoggingMarkers.INVOKER_DOCKER_CMD_TIMEOUT(args.head))

Review Comment:
   `transid.failed` already send the metric, so delete the repeated one and deleted the `INVOKER_DOCKER_CMD_TIMEOUT` method



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868983272


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerClient.scala:
##########
@@ -204,7 +205,12 @@ class DockerClient(dockerHost: Option[String] = None,
       s"running ${cmd.mkString(" ")} (timeout: $timeout)",
       logLevel = InfoLevel)
     executeProcess(cmd, timeout).andThen {
-      case Success(_) => transid.finished(this, start)
+      case Success(_) =>
+        MetricEmitter.emitHistogramMetric(
+          LogMarkerToken("docker", "runCmd", "duration", Some(args.head), Map("cmd" -> args.head))(
+            MeasurementUnit.time.milliseconds),
+          Instant.now.toEpochMilli - transid.meta.start.toEpochMilli)

Review Comment:
   Already changed to use `transid.finish` to send the metric.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] jiangpengcheng commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r882385662


##########
common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala:
##########
@@ -412,6 +412,15 @@ object LoggingMarkers {
     LogMarkerToken(invoker, "sharedPackage", counter, None, Map("path" -> path))(MeasurementUnit.none)
   def INVOKER_CONTAINERPOOL_MEMORY(state: String) =
     LogMarkerToken(invoker, "containerPoolMemory", counter, Some(state), Map("state" -> state))(MeasurementUnit.none)
+  def INVOKER_CONTAINERPOOL_CONTAINER(state: String, tags: Option[Map[String, String]] = None) = {
+    var map = Map("state" -> state)
+    tags.foreach { mapTags =>
+      for ((k, v) <- mapTags) {
+        map += (k -> v)
+      }
+    }
+    LogMarkerToken(invoker, "containerPoolContainer", counter, Some(state), map)(MeasurementUnit.none)

Review Comment:
   we can use ` tags.map(Map("state" -> "state") ++: _).getOrElse(map)` here



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] bdoyle0182 commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868854600


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala:
##########
@@ -111,6 +111,14 @@ class FunctionPullingContainerPool(
     MetricEmitter
       .emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("prewarmed"), memoryConsumptionOf(prewarmedPool))
     MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_MEMORY("max"), poolConfig.userMemory.toMB)
+    val prewarmedSize = prewarmedPool.size
+    val busySize = busyPool.size
+    val warmedSize = warmedPool.size
+    val allSize = prewarmedSize + busySize + warmedSize
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("prewarmed"), prewarmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("busy"), busySize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("warmed"), warmedSize)
+    MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("all"), allSize)

Review Comment:
   Yep this a great metric addition and needed. Should we include namespace and action as tags here as well for the warmed containers? If needed in a separate metric. I think it's useful as well to know what invokers a namespace and actions are running on. 



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] bdoyle0182 commented on pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
bdoyle0182 commented on PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#issuecomment-1121978924

   LGTM other than my one comment


-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] upgle commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
upgle commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868874611


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala:
##########
@@ -176,6 +183,11 @@ class KubernetesClient(
         waitForPod(namespace, createdPod, start.start, config.timeouts.run)
           .map { readyPod =>
             transid.finished(this, start, logLevel = InfoLevel)
+            val t1 = System.currentTimeMillis()
+            MetricEmitter.emitHistogramMetric(
+              LogMarkerToken("kubeapi", "create", "duration", Some("create"), Map("cmd" -> "create"))(
+                MeasurementUnit.time.milliseconds),

Review Comment:
   `transid.finished` method already sends Histogram metric here. What's the difference?
   
   https://github.com/apache/openwhisk/blob/99fb2c59e264d820d6b95a6319b7882f19aead4d/common/scala/src/main/scala/org/apache/openwhisk/common/TransactionId.scala#L125



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] ningyougang commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
ningyougang commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r868982635


##########
core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala:
##########
@@ -176,6 +183,11 @@ class KubernetesClient(
         waitForPod(namespace, createdPod, start.start, config.timeouts.run)
           .map { readyPod =>
             transid.finished(this, start, logLevel = InfoLevel)
+            val t1 = System.currentTimeMillis()
+            MetricEmitter.emitHistogramMetric(
+              LogMarkerToken("kubeapi", "create", "duration", Some("create"), Map("cmd" -> "create"))(
+                MeasurementUnit.time.milliseconds),

Review Comment:
   @upgle ,good found, has no any difference, i already removed the duplicated one, just keep `transid.finish` to send the metric.



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [openwhisk] jiangpengcheng commented on a diff in pull request #5222: Add containerPool container histogram metric

Posted by GitBox <gi...@apache.org>.
jiangpengcheng commented on code in PR #5222:
URL: https://github.com/apache/openwhisk/pull/5222#discussion_r882385662


##########
common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala:
##########
@@ -412,6 +412,15 @@ object LoggingMarkers {
     LogMarkerToken(invoker, "sharedPackage", counter, None, Map("path" -> path))(MeasurementUnit.none)
   def INVOKER_CONTAINERPOOL_MEMORY(state: String) =
     LogMarkerToken(invoker, "containerPoolMemory", counter, Some(state), Map("state" -> state))(MeasurementUnit.none)
+  def INVOKER_CONTAINERPOOL_CONTAINER(state: String, tags: Option[Map[String, String]] = None) = {
+    var map = Map("state" -> state)
+    tags.foreach { mapTags =>
+      for ((k, v) <- mapTags) {
+        map += (k -> v)
+      }
+    }
+    LogMarkerToken(invoker, "containerPoolContainer", counter, Some(state), map)(MeasurementUnit.none)

Review Comment:
   we can use ` tags.map(Map("state" -> state) ++: _).getOrElse(Map("state" -> state)` here



##########
common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala:
##########
@@ -412,6 +412,15 @@ object LoggingMarkers {
     LogMarkerToken(invoker, "sharedPackage", counter, None, Map("path" -> path))(MeasurementUnit.none)
   def INVOKER_CONTAINERPOOL_MEMORY(state: String) =
     LogMarkerToken(invoker, "containerPoolMemory", counter, Some(state), Map("state" -> state))(MeasurementUnit.none)
+  def INVOKER_CONTAINERPOOL_CONTAINER(state: String, tags: Option[Map[String, String]] = None) = {
+    var map = Map("state" -> state)
+    tags.foreach { mapTags =>
+      for ((k, v) <- mapTags) {
+        map += (k -> v)
+      }
+    }
+    LogMarkerToken(invoker, "containerPoolContainer", counter, Some(state), map)(MeasurementUnit.none)

Review Comment:
   we can use ` tags.map(Map("state" -> state) ++: _).getOrElse(Map("state" -> state))` here



-- 
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.

To unsubscribe, e-mail: issues-unsubscribe@openwhisk.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org