Add containerPool container histogram metric #5222

ningyougang · 2022-04-24T05:23:06Z

There has container start relative metric in FunctionFullContainerPool,
It is better to provide container size histogram metric as well

Add containerPool container size histogram metric
~~Add pod creation consume time metric~~ already existed using trans.finish
~~Add docker container creation consume time metric~~ already existed using trans.finish

Description

Related issue and scope

I opened an issue to propose and discuss this change (#????)

My changes affect the following components

Types of changes

Bug fix (generally a non-breaking change which closes an issue).
Enhancement or new feature (adds new functionality).
Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

I signed an Apache CLA.
I reviewed the style guides 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.

ningyougang · 2022-04-24T07:53:02Z

...ker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala

+ val t1 = System.currentTimeMillis()
+ MetricEmitter.emitHistogramMetric(
+ LogMarkerToken("kubeapi", "create", "duration", Some("create"), Map("cmd" -> "create"))(
+ MeasurementUnit.time.milliseconds),


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

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)

Updated accordingly.

transid.finished method already sends Histogram metric here. What's the difference?

openwhisk/common/scala/src/main/scala/org/apache/openwhisk/common/TransactionId.scala

Line 125 in 99fb2c5

MetricEmitter.emitHistogramMetric(endMarker, deltaToEnd)

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

ningyougang · 2022-04-24T07:55:56Z

...src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala

+ 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)


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

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.

I guess that would apply for the memory metric as well

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

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.

codecov-commenter · 2022-04-24T08:12:43Z

Codecov Report

Merging #5222 (4b595e0) into master (4b595e0) will not change coverage.
The diff coverage is n/a.

❗ Current head 4b595e0 differs from pull request most recent head 53cbe7f. Consider uploading reports for the commit 53cbe7f to get more accurate results

@@           Coverage Diff           @@
##           master    #5222   +/-   ##
=======================================
  Coverage   44.87%   44.87%           
=======================================
  Files         238      238           
  Lines       13996    13996           
  Branches      561      561           
=======================================
  Hits         6281     6281           
  Misses       7715     7715

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b595e0...53cbe7f. Read the comment docs.

bdoyle0182 · 2022-05-10T06:25:17Z

LGTM other than my one comment

upgle · 2022-05-10T06:46:15Z

core/invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerClient.scala

+ MetricEmitter.emitHistogramMetric(
+ LogMarkerToken("docker", "runCmd", "duration", Some(args.head), Map("cmd" -> args.head))(
+ MeasurementUnit.time.milliseconds),
+ Instant.now.toEpochMilli - transid.meta.start.toEpochMilli)


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

Already changed to use transid.finish to send the metric and delete the repeated one.

upgle

It seems that the transid.finished method is already sending histogram metric. I wonder what the difference is.

upgle · 2022-05-10T06:49:01Z

...ker/src/main/scala/org/apache/openwhisk/core/containerpool/kubernetes/KubernetesClient.scala

+ val t1 = System.currentTimeMillis()
+ MetricEmitter.emitHistogramMetric(
+ LogMarkerToken("kubeapi", "create", "duration", Some("create"), Map("cmd" -> "create"))(
+ MeasurementUnit.time.milliseconds),


transid.finished method already sends Histogram metric here. What's the difference?

openwhisk/common/scala/src/main/scala/org/apache/openwhisk/common/TransactionId.scala

Line 125 in 99fb2c5

MetricEmitter.emitHistogramMetric(endMarker, deltaToEnd)

ningyougang · 2022-05-10T08:56:49Z

common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala

 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)


Obviously, the unit should be MeasurementUnit.time.milliseconds

ningyougang · 2022-05-10T08:58:12Z

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


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

bdoyle0182 · 2022-05-23T19:49:49Z

@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

ningyougang · 2022-05-26T01:21:01Z

@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

ningyougang · 2022-05-26T06:24:33Z

...src/main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala

+ }
+ val warmedPoolResultMap: mutable.Map[(String, String), Int] = mutable.Map.empty[(String, String), Int]
+ warmedPoolGroupMap.foreach { data =>
+ warmedPoolResultMap += (data._1 -> data._2.size)


There is an incompatibility problem in travis test.

So don't use mapValues here due to scala2.13's return value is MapView and scala2.12's return type is Map

can't it just remove the type annotation?

val warmedPoolMap = warmedPool groupBy { case (_, warmedData) => (warmedData.invocationNamespace, warmedData.action.toString) } mapValues (_.size) for((data, size) <- warmedPoolMap) {

It is a good solution.

jiangpengcheng · 2022-05-26T07:15:09Z

common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala

+ map += (k -> v)
+ }
+ }
+ LogMarkerToken(invoker, "containerPoolContainer", counter, Some(state), map)(MeasurementUnit.none)


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

Updated accordingly.

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

jiangpengcheng

LGTM with some minor suggestions

* Add containerPool container histogram metric * Add pod creation consume time metric * Add docker container creation consume time metric * Use transaction to calculate * Add namespace and action tags for container metric * Avoid send metric repeatedly * Fix scala 2.13 compilation error * Update according to review comment

ningyougang commented Apr 24, 2022

View reviewed changes

ningyougang force-pushed the add-container-count-metric branch from cd40b2a to 439d6d0 Compare April 24, 2022 07:59

ningyougang force-pushed the add-container-count-metric branch from 439d6d0 to 513fccd Compare May 10, 2022 05:25

bdoyle0182 approved these changes May 10, 2022

View reviewed changes

upgle reviewed May 10, 2022

View reviewed changes

ningyougang force-pushed the add-container-count-metric branch 2 times, most recently from 18b611d to 910d7de Compare May 10, 2022 08:56

ningyougang commented May 10, 2022

View reviewed changes

ningyougang closed this May 26, 2022

ningyougang reopened this May 26, 2022

ningyougang force-pushed the add-container-count-metric branch 2 times, most recently from 4b2b1ba to 45c1d32 Compare May 26, 2022 06:20

ningyougang commented May 26, 2022

View reviewed changes

jiangpengcheng reviewed May 26, 2022

View reviewed changes

ningyougang force-pushed the add-container-count-metric branch from 45c1d32 to 71e224e Compare May 26, 2022 07:27

ningyougang added 6 commits May 26, 2022 15:55

Add containerPool container histogram metric

0c193ca

Add pod creation consume time metric

db90668

Add docker container creation consume time metric

df68fc2

Use transaction to calculate

0945141

Add namespace and action tags for container metric

998414f

Avoid send metric repeatedly

374c6ba

jiangpengcheng approved these changes May 26, 2022

View reviewed changes

Fix scala 2.13 compilation error

99d5628

Update according to review comment

53cbe7f

ningyougang force-pushed the add-container-count-metric branch from 71e224e to 53cbe7f Compare May 26, 2022 08:04

ningyougang merged commit 1a6c99d into apache:master May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add containerPool container histogram metric #5222

Add containerPool container histogram metric #5222

ningyougang commented Apr 24, 2022 •

edited

Loading

ningyougang Apr 24, 2022 •

edited

Loading

upgle May 10, 2022 •

edited

Loading

ningyougang May 10, 2022

upgle May 10, 2022

ningyougang May 10, 2022

ningyougang Apr 24, 2022 •

edited

Loading

bdoyle0182 May 10, 2022

bdoyle0182 May 10, 2022

ningyougang May 10, 2022

ningyougang May 10, 2022

codecov-commenter commented Apr 24, 2022 •

edited

Loading

bdoyle0182 commented May 10, 2022

upgle May 10, 2022

ningyougang May 10, 2022 •

edited

Loading

upgle left a comment

upgle May 10, 2022

ningyougang May 10, 2022

ningyougang May 10, 2022 •

edited

Loading

bdoyle0182 commented May 23, 2022

ningyougang commented May 26, 2022

ningyougang May 26, 2022

jiangpengcheng May 26, 2022

ningyougang May 26, 2022

jiangpengcheng May 26, 2022 •

edited

Loading

ningyougang May 26, 2022

jiangpengcheng May 26, 2022

jiangpengcheng left a comment

Add containerPool container histogram metric #5222

Add containerPool container histogram metric #5222

Conversation

ningyougang commented Apr 24, 2022 • edited Loading

Description

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

ningyougang Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

upgle May 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningyougang Apr 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 24, 2022 • edited Loading

Codecov Report

bdoyle0182 commented May 10, 2022

Choose a reason for hiding this comment

ningyougang May 10, 2022 • edited Loading

Choose a reason for hiding this comment

upgle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ningyougang May 10, 2022 • edited Loading

Choose a reason for hiding this comment

bdoyle0182 commented May 23, 2022

ningyougang commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangpengcheng May 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiangpengcheng left a comment

Choose a reason for hiding this comment

ningyougang commented Apr 24, 2022 •

edited

Loading

ningyougang Apr 24, 2022 •

edited

Loading

upgle May 10, 2022 •

edited

Loading

ningyougang Apr 24, 2022 •

edited

Loading

codecov-commenter commented Apr 24, 2022 •

edited

Loading

ningyougang May 10, 2022 •

edited

Loading

ningyougang May 10, 2022 •

edited

Loading

jiangpengcheng May 26, 2022 •

edited

Loading