OpenWhisk User Events #4584

selfxp · 2019-08-16T20:57:23Z

This service connects to events topic and publishes the events to various services like Prometheus, Datadog etc via Kamon.

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.

Configures various build related steps including code coverage and scan * Enable build scan, coverage * Enable cache and scala format check * Configure scala format and format all files * Add badge

Have a single Akka http based server running to host endpoints for 1. `/metrics` - Prometheus 2. `/ping` - Health check - If the stream reading from Kafka fails then this health check would start failing. Also add a basic test infrastructure for HTTP related code paths

Capture more metrics and add more tags to monitor cold starts etc

Switch to Prometheus Java Client and make Kamon integration optional and configurable. This allows to publish metric to Prometheus by default and still provide a way to publish to any other Kamon support system via config

Extract a trait to define metric names which can be adapted per metric system conventions

Enable use of Kamon API to track common metrics which are not specific to any action. As such metrics would be bounded its safe to use Kamon Prometheus Exporter to manage them. This ensures that such metrics can be tracked via one implementation and only action specific metrics would be tracked via 2 implementations

chetanmeh · 2019-08-29T10:22:31Z

core/monitoring/user-events/src/main/resources/application.conf

+  }
+}
+
+user-events {


As a convention any OpenWhisk specific config should be placed under whisk namespace.

So move these config under whisk namespace. This would require change in code where the config is read.

style95 · 2019-09-03T07:12:51Z

This is great as we can modularize UserEvent exporter.
One question is, it looks a dedicated microservice, should this be maintained in the core repo?

selfxp · 2019-09-04T15:57:03Z

Yeah, good question @style95, there was a voting going on related to the question of where this microservice should be moved and the community decided to have it in the main OW repo.
Please see the discussion here.

style95 · 2019-09-05T00:52:28Z

@selfxp Thank you for letting me know. I missed it.

tysonnorris · 2019-09-09T16:33:04Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

+    val nameArr = name.split("/", 2)
+    (nameArr(0), nameArr(1))
+  }
+


Any reason not to split these in the Activation definition? (and avoid parsing these later etc)

tysonnorris · 2019-09-09T16:34:56Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

  override def serialize = toJson.compactPrint

  def toJson = Activation.activationFormat.write(this)
+
+  def status: String = statusCode match {


ActivationResponse already has messageForCode() - can we use that instead?

Yeah makes sense, I'll use that

tysonnorris · 2019-09-09T17:40:11Z

.../user-events/src/main/scala/org/apache/openwhisk/core/monitoring/metrics/EventConsumer.scala

+  private implicit val ec: ExecutionContext = system.dispatcher
+
+  //Record the rate of events received
+  private val activationCounter = Kamon.counter("openwhisk.userevents.global.activations")


Should we use the same metrics naming convention used in invoker/controller?
e.g.

#kamon related configuration kamon { environment { # Identifier for this service. For keeping it backward compatible setting to natch previous # statsd name service = "openwhisk-statsd" }

then metric names like "global.activations"
This would make queries/tags/etc for user events metrics more like queries/tags/etc for invoker/controller metrics.

That was done more for compatability reasons and has issues like gauge/counter etc come in name. As this is new work I think we can with approach used in current impl

style95 · 2019-09-10T02:47:35Z

.../user-events/src/main/scala/org/apache/openwhisk/core/monitoring/metrics/EventConsumer.scala

+    duration.record(a.duration.toMillis)
+  }
+
+  private def updatedSettings = settings.withProperty(ConsumerConfig.CLIENT_ID_CONFIG, id)


Would it be worth to use a unique name for each instance or leave it blank(to let Kafka define a unique name) so that we can run multiple instances for scalability and failover?

style95 · 2019-09-10T03:07:27Z

...ser-events/src/main/scala/org/apache/openwhisk/core/monitoring/metrics/OpenWhiskEvents.scala

+  def eventConsumerSettings(config: Config): ConsumerSettings[String, String] =
+    ConsumerSettings(config, new StringDeserializer, new StringDeserializer)
+      .withGroupId("kamon")
+      .withProperty(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest")


How about using the configuration file to make it possible for downstream to decide the default behavior?

style95 · 2019-09-10T03:37:58Z

.../user-events/src/main/scala/org/apache/openwhisk/core/monitoring/metrics/KamonRecorder.scala

+  private val activationMetrics = new TrieMap[String, ActivationKamonMetrics]
+  private val limitMetrics = new TrieMap[String, LimitKamonMetrics]
+
+  override def processActivation(activation: Activation, initiatorNamespace: String): Unit = {


Does initiatorNamespace mean here a user's namespace who invokes shared actions or web actions?

perhaps this pr #4609 makes it easier.

yes exactly, initiator would be the namespace that makes the invocation

style95 · 2019-09-10T03:53:09Z

...ser-events/src/main/scala/org/apache/openwhisk/core/monitoring/metrics/OpenWhiskEvents.scala

+    val metricConfig = loadConfigOrThrow[MetricConfig](config, "whisk.user-events")
+
+    val prometheusRecorder = PrometheusRecorder(prometheusReporter)
+    val recorders = if (metricConfig.enableKamon) Seq(prometheusRecorder, KamonRecorder) else Seq(prometheusRecorder)


While I am ok with the current version, I have one small question.
Can we make the default recorder and reporter configurable as well?
In some downstream, they may want to use KamonRecorder with some reporters as default instead of Prometheus.
In such a case, the Prometheus recorder will still record metrics without any use.

👍 I'll create an issue to keep track of this

rabbah

generally LGTM.

rabbah · 2019-09-12T10:08:28Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

  override def serialize = toJson.compactPrint

  def toJson = Activation.activationFormat.write(this)
+
+  def status: String = statusCode match {


rabbah · 2019-09-12T10:09:27Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

+    }
+
+    override def read(json: JsValue): Duration = json match {
+      case JsNumber(n) if n <= 0 => Duration.Zero


fwiw this would suggest a bug.

rabbah · 2019-09-12T10:09:44Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

+
+    override def read(json: JsValue): Duration = json match {
+      case JsNumber(n) if n <= 0 => Duration.Zero
+      case JsNumber(n)           => toDuration(n.longValue())


nit; drop () in longValue() and in isFinite() above.

rabbah · 2019-09-12T10:10:27Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

  def parse(msg: String) = Try(activationFormat.read(msg.parseJson))
+
+  val statusSuccess = "success"


these would get removed as well if you reuse existing code to map status code to a string.

rabbah · 2019-09-12T10:13:45Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

+   * Extract namespace and action from name
+   * ex. whisk.system/apimgmt/createApi -> (whisk.system, apimgmt/createApi)
+   */
+  def getNamespaceAndActionName(name: String): (String, String) = {


it may be helpful to use EntityPath(name).toFullyQualifiedEntityName instead as there could be an optional a leading slash... up to you.

rabbah · 2019-09-12T10:16:57Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

@@ -223,6 +256,15 @@ object Activation extends DefaultJsonProtocol {
      "memory",
      "causedBy")


all these field names are defined in WhiskActivation - if there are some not defined there, they should be - this keeps the list in one canonical place. While not related to this PR, we should fix this either here or independently.

👍 I'll create an issue for this

rabbah · 2019-09-12T10:19:19Z

.../user-events/src/main/scala/org/apache/openwhisk/core/monitoring/metrics/KamonRecorder.scala

+  private val activationMetrics = new TrieMap[String, ActivationKamonMetrics]
+  private val limitMetrics = new TrieMap[String, LimitKamonMetrics]
+
+  override def processActivation(activation: Activation, initiatorNamespace: String): Unit = {


perhaps this pr #4609 makes it easier.

rabbah · 2019-09-12T10:20:51Z

...events/src/main/scala/org/apache/openwhisk/core/monitoring/metrics/PrometheusEventsApi.scala

+  val textV4: ContentType = ContentType.parse("text/plain; version=0.0.4; charset=utf-8").right.get
+}
+
+class PrometheusEventsApi(consumer: EventConsumer, prometheus: PrometheusExporter) {


how is this service authenticated/secured?

it's not, I guess it depends on the environment on where it's being deployed. In openwhisk-deploy-kube I have a PR where the service would be exposed through the gateway (#520), and so the authentication strategy could be defined there

sven-lange-last · 2019-09-17T16:52:03Z

core/monitoring/user-events/README.md

+
+# OpenWhisk User Events
+
+This service connects to `events` topic and publishes the events to various services like Prometheus, Datadog etc via Kamon. Refer to [user specific metrics][1] on how to enable them.


I suggest to extend https://github.com/apache/openwhisk/blob/master/docs/metrics.md#user-specific-metrics to point to the metrics emitter provided by this PR.

👍 yes good idea, adding a reference

chetanmeh · 2019-09-19T09:03:06Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

  override def serialize = toJson.compactPrint
+  def entityPath = EntityPath(name).toFullyQualifiedEntityName


Nit: Define return type for public methods. Like entityPath

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

chetanmeh · 2019-09-19T09:12:01Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/ActivationResult.scala

-      case ApplicationError => "application error"
-      case DeveloperError   => "action developer error"
-      case WhiskError       => "whisk internal error"
+      case 0 => ActivationResponse.statusSuccess


Better to retain use of existing constants.

chetanmeh · 2019-09-19T09:16:48Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/ActivationResult.scala

    }
  }

+  protected[core] def messageForCode(code: Int) = {


nit: messageForCode is called in critical frequent use path. So to avoid unnecessary string computation it may be better to also keep the computed values as val and just reuse them

chetanmeh · 2019-09-19T09:19:28Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskActivation.scala

@@ -145,6 +145,10 @@ object WhiskActivation
  val conductorAnnotation = "conductor"
  val timeoutAnnotation = "timeout"

+  val memoryAnnotation = "memory"


These are not annotations. So keep the name without annotation suffix

chetanmeh · 2019-09-19T11:16:26Z

common/scala/src/main/scala/org/apache/openwhisk/core/connector/Message.scala

-      "conductor",
-      "memory",
-      "causedBy")
+      WhiskActivation.statusCodeAnnotation,


This can be avoided as it does not serve much purpose to use those annotations value as constant (some are not annotation also like memory). So it would be fine to revert to preexisting way

chetanmeh · 2019-09-19T11:17:26Z

common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskActivation.scala

@@ -82,8 +82,8 @@ case class WhiskActivation(namespace: EntityPath,
      if (end != Instant.EPOCH) {
        Map(
          "end" -> end.toJson,
-          "duration" -> (duration getOrElse (end.toEpochMilli - start.toEpochMilli)).toJson,
-          "statusCode" -> response.statusCode.toJson)
+          WhiskActivation.durationAnnotation -> (duration getOrElse (end.toEpochMilli - start.toEpochMilli)).toJson,


This change of using existing constants can be avoided. Using string literals is fine here

upgle · 2019-09-24T04:46:49Z

...-events/src/test/scala/org/apache/openwhisk/core/monitoring/metrics/KamonRecorderTests.scala

+      TestReporter.histogram(initTimeMetric, actionWithCustomPackage).size shouldBe 1
+      TestReporter.histogram(durationMetric, actionWithCustomPackage).size shouldBe 1
+
+//      // Default package


there is duplicated annotation

chetanmeh · 2019-09-25T04:32:47Z

Thanks to all the reviewers for the valuable feedback and @selfxp for following up on them. This PR is now merged and OpenWhisk setups can now leverage this service to expose action level metrics to end users

The user event service enables aggregating the metric events sent on `events` topic and expose them as Prometheus (or Kamon) metrics. Out of the box dashboards are provided for the Grafana/Prometheus mode which provide detailed insights on performance metrics at cluster/namespace/action level.

chetanmeh and others added 30 commits October 25, 2018 11:11

Initial commit

b8be6a3

Initial commit - Gradle support

79472f5

Initial commit - Compile with Scala

fd045a6

Enable Kamon and package as application

79874c9

Exclude dependencies which are not required

2606557

Copy the minimum EventMessage model from OpenWhisk code

9f76657

Initial working implementation

b581155

Default logging config

5b53b14

Lookup KAFKA_HOST from env

bd5198d

Change launcher class post refactoring

4ed0f20

Docker build

05bba0a

Comment to indicate the classes here are from OpenWhisk

dc647f0

Add remade and makefile

43ccd3c

Update Readme

196e17e

Prepare application for publishing to a public repo

82d3e8a

Add LICENSE file

3448f7c

Update README to remove unnecessary information

cddb3a8

Add docker-compose for prometheus and grafana

6abf7b3

Add build targets and update readme

8d54bdb

Remove some of the contributing guidelines parts

9e85892

Update docker image name

2387fc9

Enable Travis builds (apache#2)

b8e5786

Enable build scan and coverage (apache#3)

65a00ef

Configures various build related steps including code coverage and scan * Enable build scan, coverage * Enable cache and scala format check * Configure scala format and format all files * Add badge

Add additional tags to the existing metrics (apache#1)

2355539

Capture more metrics and add more tags to monitor cold starts etc

Use Prometheus Java Client (apache#5)

b8f83e2

Switch to Prometheus Java Client and make Kamon integration optional and configurable. This allows to publish metric to Prometheus by default and still provide a way to publish to any other Kamon support system via config

Match metric names per convention (apache#9)

d926df7

Extract a trait to define metric names which can be adapted per metric system conventions

Support for pushing a docker image to a registry (apache#13)

65dabf5

Fix issue of not being able to execute docker_push.sh (apache#14)

355de66

Reconfigure logback

89bff99

chetanmeh reviewed Aug 29, 2019

View reviewed changes

Move specific config under the whisk namespace

f04275a

selfxp mentioned this pull request Aug 31, 2019

Add support for user-events monitoring apache/openwhisk-deploy-kube#520

Merged

tysonnorris reviewed Sep 9, 2019

View reviewed changes

style95 reviewed Sep 10, 2019

View reviewed changes

rabbah approved these changes Sep 12, 2019

View reviewed changes

Use already existing messageForCode def

4965397

sven-lange-last reviewed Sep 17, 2019

View reviewed changes

selfxp and others added 6 commits September 17, 2019 17:10

Use EntityPath(name).toFullyQualifiedEntityName instead of path split

a38c668

Use field names defined in WhiskActivation

cc6329d

Add user-events reference in readme

e8d9234

Merge branch 'master' into ow-monitoring

fa27d59

Make group.id and auto.offset.reset configurable

8081a1f

Merge remote-tracking branch 'origin/ow-monitoring' into ow-monitoring

ff9a141

chetanmeh reviewed Sep 19, 2019

View reviewed changes

Define return type for public methods

9876c4c

upgle reviewed Sep 24, 2019

View reviewed changes

selfxp added 2 commits September 24, 2019 08:22

Use already defined constants

38b3174

Remove duplicated annotation

09bd41e

chetanmeh merged commit 3e89aa5 into apache:master Sep 25, 2019

		def parse(msg: String) = Try(activationFormat.read(msg.parseJson))

		val statusSuccess = "success"

		@@ -223,6 +256,15 @@ object Activation extends DefaultJsonProtocol {
		"memory",
		"causedBy")


		# OpenWhisk User Events

		This service connects to `events` topic and publishes the events to various services like Prometheus, Datadog etc via Kamon. Refer to [user specific metrics][1] on how to enable them.

		override def serialize = toJson.compactPrint
		def entityPath = EntityPath(name).toFullyQualifiedEntityName

OpenWhisk User Events #4584

OpenWhisk User Events #4584

Conversation

selfxp commented Aug 16, 2019 • edited

Related issue and scope

My changes affect the following components

Types of changes

Checklist:

Choose a reason for hiding this comment

style95 commented Sep 3, 2019

selfxp commented Sep 4, 2019

style95 commented Sep 5, 2019

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

tysonnorris Sep 9, 2019 • edited

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

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

rabbah left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfxp Sep 13, 2019 • edited

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chetanmeh commented Sep 25, 2019

selfxp commented Aug 16, 2019 •

edited

tysonnorris Sep 9, 2019 •

edited

selfxp Sep 13, 2019 •

edited