Support time-series DB using Kamon Tag #3343

upgle · 2018-02-24T07:21:28Z

This PR is for supporting time-series database using Kamon Tag like OpenTSDB, Datadog.

and I also added granular metric again which was removed from #3284 PR. (but it is optional, default is False) because we need this feature for tracking activities per namespace, and already have built up enough infrastructure.

Reference

http://opentsdb.net/docs/build/html/user_guide/writing.html

OpenTSDB handles things a bit differently by introducing the idea of 'tags'. Each time series still has a 'metric' name, but it's much more generic, something that can be shared by many unique time series. Instead, the uniqueness comes from a combination of tag key/value pairs that allows for flexible queries with very fast aggregations.

https://docs.datadoghq.com/agent/tagging/

Tagging is used throughout the Datadog product to make it easier to subset and query the machines and metrics that you have to monitor.

Metrics tested in real environment.

* It is tested on both OpenTSDB and Graphite

1. OpenTSDB (Tag based aggregation)

In OpenTSDB, you can use regexable aggregation query with tag only.
e.g. host=regexp(web[0-9]+.lax.mysite.com)

2. Graphite (Metric name based aggregation)

In Graphite, you can use regexable aggregation query with a metric name only.
e.g. invoker_containerStart_*

csantanapr · 2018-02-24T11:40:23Z

If it’s optional and and is false by default I don’t see an issue to add in the grand scheme of things.
👍

markusthoemmes

Thanks for adding this! 🎉

Got a lengthy question on the API design here, let's see what you think.

markusthoemmes · 2018-02-24T12:10:37Z

common/scala/src/main/scala/whisk/common/Logging.scala

-        .counter(token.toString)
-        .increment(1)
+      if (token.tags.nonEmpty) metrics.counter(token.toString, token.tags.get).increment(1)
+      else metrics.counter(token.toString).increment(1)


Should be:

token.tags match { case Some(tags) => metrics.counter(token.toString, token.tags).increment(1) case None => metrics.counter(token.toString).increment(1) }

Same below.

Edit: Might be ignorable, see lengthy comment.

markusthoemmes · 2018-02-24T12:12:33Z

common/scala/src/main/scala/whisk/common/Logging.scala

-  def INVOKER_CONTAINER_START(containerState: String) =
-    LogMarkerToken(invoker, s"container_start_${containerState}", count)
-  def INVOKER_KUBECTL_CMD(cmd: String) = LogMarkerToken(invoker, s"kubectl.$cmd", start)
+  def INVOKER_DOCKER_CMD(cmd: String) = LogMarkerToken(invoker, "docker", start, Map("cmd" → cmd))


Please remove all ASCII arrows and replace them with -> (memo to self: automate this check)

markusthoemmes · 2018-02-24T12:22:49Z

common/scala/src/main/scala/whisk/common/Logging.scala

 }

 private object Emitter {
  val timeFormat = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'").withZone(ZoneId.of("UTC"))
 }

-case class LogMarkerToken(component: String, action: String, state: String) {
+case class LogMarkerToken(component: String, action: String, state: String, tags: Option[Map[String, String]] = None) {


I've been looking through all the PR, and it feels a bit unclear to me to grasp all the branches on when something will be logged/sent and how. Maybe we can simplify:

What do you think about creating

LogMarkerToken(component: String, action: String, state: String, macroTags: Map[String, String] = Map.empty, microTags: Map[String, String] = Map.empty)

Then we could have

override def toString = component + "_" + action + "_" + state override def toStringWithTags = component + "_" + action + macroTags.mkString(".", ".", "") + "_" + state

And call them in the specific call-site accordingly:

def emitCounterMetric(token: LogMarkerToken): Unit = { if (TransactionId.metricsKamon) { if (TransactionId.metricsKamonTags) { metrics.counter(token.toString, token.macroTags ++ token.microTags).increment(1) } else { metrics.counter(token.toStringWithTags).increment(1) } } }

We maybe don't even need the granular option then. If the backend supports tags, we're fine anyway, right? We just need to make sure we only log the macro tags if the backend does not support "true" tags.

WDYT?

Yeah, I agree with this all and think it is a very clear solution.
I will modify the code, test it, and then commit it again.

Thank you for your kind review!

@markusthoemmes
I changed the code, but I'm worried also the scaleable issue that occurs when using a low-capacity and low-performance Kamon-backend that supports tags.

Should we make a granular option again?
What do you think..?

markusthoemmes · 2018-02-24T13:10:55Z

common/scala/src/main/scala/whisk/common/Logging.scala

 }

 private object Emitter {
  val timeFormat = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'").withZone(ZoneId.of("UTC"))
 }

-case class LogMarkerToken(component: String, action: String, state: String) {
+case class LogMarkerToken(component: String,


A ScalaDoc comment here would be great, stating that macroTags should be used for tags with a bounded number of permutations only while microTags can be used for whatever granularity you might need.

markusthoemmes · 2018-02-24T13:12:32Z

common/scala/src/main/scala/whisk/http/BasicHttpService.scala

-      val token = LogMarkerToken("http", s"${m.toLowerCase}.${res.status.intValue}", LoggingMarkers.count)
+      val token = LogMarkerToken(
+        "http",
+        s"${m.toLowerCase}",


Doesn't need string interpolation anymore. Just m.toLowerCase should suffice.

markusthoemmes · 2018-02-24T13:13:06Z

common/scala/src/main/scala/whisk/common/Logging.scala

+  def INVOKER_CONTAINER_START(containerState: String, namespaceName: String, actionName: String) =
+    LogMarkerToken(
+      invoker,
+      s"containerStart",


No string interpolation needed. Just containerStart is fine.

Modified. thank you

upgle · 2018-02-26T07:01:13Z

@markusthoemmes could you please review again?

markusthoemmes

Some more nits. We shouldn't entangle the LogMarkerTokens with a global value.

@mhenke1 could you please review the change in INVOKER_CONTAINER_START?

It changes from container_start_cold to containerStart.cold

markusthoemmes · 2018-02-26T07:36:11Z

common/scala/src/main/scala/whisk/common/Logging.scala

  override def toString() = component + "_" + action + "_" + state

+  // folderLeft is used instead of mkString(".", ".", ""),
+  // because Map.empty.mkString returns "."
+  def toStringWithTags = component + "_" + action + getTags.values.foldLeft("")((a, v) => a + "." + v) + "_" + state


You can simplify to

def toStringWithTags(includeMicroTags: Boolean) = { val tags = if(includeMicroTags) macroTags ++ microTags else macroTags val tagString = tags.values.foldLeft("")(_ + "." + _) // graceful handling of an empty map component + "_" + action + tagString + "_" + state }

Note: I'd remove the entanglement of this instance to some arbitrary global value and pass it in instead for better control (in tests for instance).

markusthoemmes · 2018-02-26T07:36:43Z

common/scala/src/main/scala/whisk/common/Logging.scala

+      if (TransactionId.metricsKamonTags) {
+        metrics.counter(token.toString, token.getTags).increment(1)
+      } else {
+        metrics.counter(token.toStringWithTags).increment(1)


Changes to

metrics.counter(token.toStringWithTags(includeMicroTags = false)).increment(1)

markusthoemmes · 2018-02-26T07:37:13Z

common/scala/src/main/scala/whisk/common/Logging.scala

-        .counter(token.toString)
-        .increment(1)
+      if (TransactionId.metricsKamonTags) {
+        metrics.counter(token.toString, token.getTags).increment(1)


Changes to

metrics.counter(token.toString, if(TransactionId.granularMetric) token.allTags else token.macroTags).increment(1)

markusthoemmes · 2018-02-26T07:42:12Z

common/scala/src/main/scala/whisk/common/Logging.scala

+  // folderLeft is used instead of mkString(".", ".", ""),
+  // because Map.empty.mkString returns "."
+  def toStringWithTags = component + "_" + action + getTags.values.foldLeft("")((a, v) => a + "." + v) + "_" + state
+  def getTags = if (TransactionId.granularMetric) macroTags ++ microTags else macroTags


How about def allTags = macroTags ++ microTags and use it from the outside?

mhenke1 · 2018-02-26T08:02:16Z

common/scala/src/main/scala/whisk/common/Logging.scala

+ * @param component Component like invoker, controller, and docker. It is defined in LoggingMarkers.
+ * @param action Action of the component.
+ * @param state State of the action.
+ * @param macroTags macroTags should be used for tags with a bounded number of permutations.


Please add an example for such a bounded tag (e.g. http staus code)

mhenke1

lgtm (please see one comment regarding parameter explaination)

markusthoemmes

LGTM

markusthoemmes · 2018-02-28T07:30:44Z

@upgle it's taking me so long because there is a test failure in RuncClient tests. It is currently not run by travis due to a bug (will be resolved shoon #3367). Can you please take a look?

upgle · 2018-02-28T14:01:38Z

@markusthoemmes has it been resolved?

markusthoemmes · 2018-02-28T15:14:32Z

@upgle it's merged yep, you can now rebase and try if the test passes (i think it won't).

upgle · 2018-03-01T03:28:28Z

The test is passed, and some test code has modified to use log parser which is changed the signature.
now if there are some tags, log parser for unit test uses LogMarkerToken as a hint to parsing.
please review again. @markusthoemmes

upgle · 2018-03-01T14:08:30Z

conflicts have been resolved.

markusthoemmes · 2018-03-01T14:31:30Z

PG3 1939 🔵

csantanapr requested review from cbickel and markusthoemmes February 24, 2018 11:40

csantanapr assigned cbickel and markusthoemmes Feb 24, 2018

markusthoemmes requested changes Feb 24, 2018

View reviewed changes

markusthoemmes reviewed Feb 24, 2018

View reviewed changes

markusthoemmes requested changes Feb 26, 2018

View reviewed changes

cbickel removed their request for review February 26, 2018 07:50

cbickel removed their assignment Feb 26, 2018

mhenke1 reviewed Feb 26, 2018

View reviewed changes

markusthoemmes approved these changes Feb 26, 2018

View reviewed changes

markusthoemmes added needs-review awaits-feedback A reviewer has asked a question and/or asked for a change. reviewed Review for this PR is finished. It is mergeable from a review's perspective. and removed needs-review labels Feb 28, 2018

upgle changed the title ~~Support time-series DB using Kamon Tag, and optional granular metric.~~ Support time-series DB using Kamon Tag Mar 1, 2018

upgle added 5 commits March 1, 2018 22:12

Modify MetricEmitter to support tagging

85a469a

Add kamon tags configuration.

ef7f3ba

Change environment variables.

d999a77

Update metrics guide

9e7e9ee

Change docker metric token to use tag

12ae7cc

upgle added 14 commits March 1, 2018 22:12

Modify HTTP token to use tag

4f3ec97

Fix scalafmt test error

e08dfdf

Modify to use macro, micro tag

94b3dd7

Reformat string variables

dae4473

Add scalaDoc

963600b

Reformat string constant

6236f9e

Fix scalafmt

eb53701

Add granular metric option

3235298

Modify tag name to short

bc3b177

Change mkString to use foldLeft

8b1ca4b

Code cleanup

50cf802

Add annotation

863c1c8

Change LogMarkerToken signature

e40bb47

Remove granular option.

07c774a

markusthoemmes added pgapproved Pipeline has approved this change. and removed awaits-feedback A reviewer has asked a question and/or asked for a change. labels Mar 1, 2018

markusthoemmes merged commit 6f1a445 into apache:master Mar 1, 2018

chetanmeh mentioned this pull request Jul 30, 2018

Provide option to support static metric names #3917

Closed

BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019

Support tags in metrics for finer granularity. (apache#3343)

3dd1a21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support time-series DB using Kamon Tag #3343

Support time-series DB using Kamon Tag #3343

upgle commented Feb 24, 2018 •

edited

csantanapr commented Feb 24, 2018

markusthoemmes left a comment

markusthoemmes Feb 24, 2018

markusthoemmes Feb 24, 2018

markusthoemmes Feb 24, 2018

markusthoemmes Feb 24, 2018

upgle Feb 24, 2018

upgle Feb 24, 2018

markusthoemmes Feb 24, 2018

markusthoemmes Feb 24, 2018

markusthoemmes Feb 24, 2018

upgle Feb 24, 2018

upgle commented Feb 26, 2018

markusthoemmes left a comment

markusthoemmes Feb 26, 2018

markusthoemmes Feb 26, 2018

markusthoemmes Feb 26, 2018

markusthoemmes Feb 26, 2018

mhenke1 Feb 26, 2018

mhenke1 left a comment

markusthoemmes left a comment

markusthoemmes commented Feb 28, 2018

upgle commented Feb 28, 2018

markusthoemmes commented Feb 28, 2018

upgle commented Mar 1, 2018 •

edited

upgle commented Mar 1, 2018

markusthoemmes commented Mar 1, 2018

Support time-series DB using Kamon Tag #3343

Support time-series DB using Kamon Tag #3343

Conversation

upgle commented Feb 24, 2018 • edited

Reference

Metrics tested in real environment.

1. OpenTSDB (Tag based aggregation)

2. Graphite (Metric name based aggregation)

csantanapr commented Feb 24, 2018

markusthoemmes 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

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

upgle commented Feb 26, 2018

markusthoemmes 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

mhenke1 left a comment

Choose a reason for hiding this comment

markusthoemmes left a comment

Choose a reason for hiding this comment

markusthoemmes commented Feb 28, 2018

upgle commented Feb 28, 2018

markusthoemmes commented Feb 28, 2018

upgle commented Mar 1, 2018 • edited

upgle commented Mar 1, 2018

markusthoemmes commented Mar 1, 2018

upgle commented Feb 24, 2018 •

edited

upgle commented Mar 1, 2018 •

edited