Skip to content

Add containerPool container histogram metric #5222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 27, 2022
+22 −5
Merged

Add containerPool container histogram metric #5222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0c193ca
Add containerPool container histogram metric
ningyougang Apr 24, 2022
db90668
Add pod creation consume time metric
ningyougang Apr 24, 2022
df68fc2
Add docker container creation consume time metric
ningyougang Apr 24, 2022
0945141
Use transaction to calculate
ningyougang May 10, 2022
998414f
Add namespace and action tags for container metric
ningyougang May 10, 2022
374c6ba
Avoid send metric repeatedly
ningyougang May 10, 2022
99d5628
Fix scala 2.13 compilation error
ningyougang May 26, 2022
53cbe7f
Update according to review comment
ningyougang May 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions common/scala/src/main/scala/org/apache/openwhisk/common/Logging.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,10 @@ 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) = {
val map = Map("state" -> state) ++: tags.getOrElse(Map.empty)
LogMarkerToken(invoker, "containerPoolContainer", counter, Some(state), map)(MeasurementUnit.none)
Copy link
Contributor

@jiangpengcheng jiangpengcheng May 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// System overload and random invoker assignment
val MANAGED_SYSTEM_OVERLOAD =
Expand Down Expand Up @@ -493,12 +497,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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously, the unit should be MeasurementUnit.time.milliseconds

def INVOKER_CONTAINER_START(containerState: String, invocationNamespace: String, namespace: String, action: String) =
LogMarkerToken(
invoker,
Expand Down
3 changes: 1 addition & 2 deletions .../invoker/src/main/scala/org/apache/openwhisk/core/containerpool/docker/DockerClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import scala.util.Try
import akka.event.Logging.{ErrorLevel, InfoLevel}
import pureconfig._
import pureconfig.generic.auto._
import org.apache.openwhisk.common.{Logging, LoggingMarkers, MetricEmitter, TransactionId}
import org.apache.openwhisk.common.{Logging, LoggingMarkers, TransactionId}
import org.apache.openwhisk.core.ConfigKeys
import org.apache.openwhisk.core.containerpool.ContainerId
import org.apache.openwhisk.core.containerpool.ContainerAddress
Expand Down Expand Up @@ -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))
Copy link
Contributor Author

@ningyougang ningyougang May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case Failure(t) => transid.failed(this, start, t.getMessage, ErrorLevel)
}
}
Expand Down
16 changes: 16 additions & 0 deletions .../main/scala/org/apache/openwhisk/core/containerpool/v2/FunctionPullingContainerPool.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,23 @@ 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 warmedPoolMap = warmedPool groupBy {
case (_, warmedData) => (warmedData.invocationNamespace, warmedData.action.toString)
} mapValues (_.size)
for ((data, size) <- warmedPoolMap) {
val tags: Option[Map[String, String]] = Some(Map("namespace" -> data._1, "action" -> data._2))
MetricEmitter.emitHistogramMetric(LoggingMarkers.INVOKER_CONTAINERPOOL_CONTAINER("warmed", tags), 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("all"), allSize)
Copy link
Contributor Author

@ningyougang ningyougang Apr 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that would apply for the memory metric as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

})

// Key is ColdStartKey, value is the number of cold Start in minute
Expand Down