Add graphite support #114

yazgoo · 2020-01-22T10:08:28Z

This allows to specify a graphite output via the following config:

  graphite-host = "localhost"
  graphite-port = 2003

This translates prometheus labels into a graphite metric path.

lightbend-cla-validator · 2020-01-22T10:08:32Z

Hi @yazgoo,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

lightbend-cla-validator · 2020-01-22T10:17:38Z

Hi @yazgoo,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

yazgoo · 2020-01-22T10:21:58Z

I signed the CLA

seglo · 2020-01-24T16:14:24Z

Thanks @yazgoo. I'll review this shortly.

seglo

Thanks a lot for diving into this. This project was always meant as a Prometheus project, but I don't have any particular objection to supporting Graphite. Instead of mixing the implementation into the PrometheusEndpointSink I would ask that you create a new GraphiteEndpointSink. Since there will now be two available sinks some thought should be given to the design on how to plug and play one (or both?) if it makes sense. We should also try to have at least testing in place, maybe even some integration tests. Are you open to iterating on this further?

yazgoo · 2020-01-28T14:57:57Z

Thanks @seglo, 👌 for iterating on this.

👌 graphite: add periodInSecond conf and stop 🎨 remove graphite bridge 🥅 handle exceptions

yazgoo · 2020-01-30T08:46:16Z

@seglo PR ready for review

creating one metric sink dedicated to graphite

seglo

The reporter abstraction is looking really good! I made some comments about how I would like to see a reporter's creation more config generated.

Please make some updates to the README too. You don't need to mention Graphite with every mention of Prometheus. A separate section would be alright. You should also update the configuration section in the "Run Standalone" section to document graphite config.

Do you use the Helm chart for deployment to Kubernetes? If so, can you update that as well? If not, I'll do it.

seglo · 2020-02-09T18:57:48Z

src/main/scala/com/lightbend/kafkalagexporter/GraphiteEndpointSink.scala

+  def apply(metricWhitelist: List[String], clusterGlobalLabels: ClusterGlobalLabels,
+            graphiteConfig: Option[GraphiteConfig]): MetricsSink = {
+    Try(new GraphiteEndpointSink(metricWhitelist, clusterGlobalLabels, graphiteConfig))
+      .fold(t => throw new Exception("Could not create Prometheus Endpoint", t), sink => sink)


Update failure message

seglo · 2020-02-09T19:12:28Z

src/main/scala/com/lightbend/kafkalagexporter/GraphiteEndpointSink.scala

+    }
+  }
+
+  def metricNameToGraphiteMetricName(metricValue: MetricValue): String = {


I'm not sure I understand what's happening here. The metrics are already snakecase. Can you add a scaladoc comment with an example of the conversion?

added @example

seglo · 2020-02-09T19:14:37Z

src/main/scala/com/lightbend/kafkalagexporter/MainApp.scala

+          Metrics.definitions, appConfig.metricWhitelist, appConfig.clustersGlobalLabels(), server, CollectorRegistry.defaultRegistry
+        ))
+      ),
+      KafkaClusterManager.NamedCreator(


I'd prefer to only create the graphite-lag-reporter if its config exists, otherwise it seems like a waste to create it at all and report all metrics to it.

seglo · 2020-02-09T19:21:10Z

src/main/scala/com/lightbend/kafkalagexporter/AppConfig.scala

@@ -18,6 +18,11 @@ import scala.util.Try
 object AppConfig {


Now that we have more than 1 reporter lets generate them based on configuration and only create them when necessary. Example application.conf:

kafka-lag-exporter { reporters { prometheus { port = 8000 } graphite { host = "foo" port = 9999 } } }

seglo · 2020-02-09T19:22:12Z

src/main/scala/com/lightbend/kafkalagexporter/AppConfig.scala

@@ -134,6 +147,7 @@ final case class AppConfig(pollInterval: FiniteDuration, lookupTableSize: Int, p
       |Prometheus metrics whitelist: [${metricWhitelist.mkString(", ")}]


We can rename this to "metrics whitelist"

Add an indented section for prometheus like you did for Graphite

Prometheus: Port: $prometheusPort

seglo · 2020-02-09T19:27:14Z

src/main/scala/com/lightbend/kafkalagexporter/GraphiteEndpointSink.scala

+    }
+  }
+
+  override def remove(m: RemoveMetric): Unit = {


This is used in prometheus to unpublish a metric for a consumer group that disappears. If a consumer group isn't reported for awhile and we don't push metrics for it to Graphite, what would the Graphite metric for that now removed consumed group look like? Do we need to worry about unpublishing metrics?

I don't think in graphite you have to unregister a metric since graphite is push based.
When the max retention is reached the metric will be totally removed by graphite itself.

seglo · 2020-02-09T19:33:06Z

src/main/scala/com/lightbend/kafkalagexporter/AppConfig.scala

+      for (host <- Try(c.getString("graphite-host"));
+           port <- Try(c.getInt("graphite-port")),
+             ) yield GraphiteConfig(
+               host, port, Try(c.getString("graphite-prefix")).toOption)).toOption
    val pollInterval = c.getDuration("poll-interval").toScala
    val lookupTableSize = c.getInt("lookup-table-size")
    val port = c.getInt("port")


Support getting the prometheus port from here, and from the new report configuration section I mentioned above. Use kafka-lag-exporter.port as the default and override with kafka-lag-exporter.prometheus.port if it exists. This will maintain backwards compatibility.

Please update the reference.conf to include the kafka-lag-exporter.reporters.prometheus section.

yazgoo · 2020-02-11T10:28:05Z

Do you use the Helm chart for deployment to Kubernetes? If so, can you update that as well? If not, I'll do it.

Unfortunately I'm not using kafka-lag-exporter w/ kubernetes :)

yazgoo · 2020-02-19T14:16:15Z

ready for review @seglo

seglo

LGTM. Just a few minor docs fixes to add.

seglo · 2020-02-27T18:45:45Z

README.md

@@ -224,20 +224,32 @@ To run the project in standalone mode you must first define a configuration `app
 contain at least connection info to your Kafka cluster (`kafka-lag-exporter.clusters`). All other configuration has
 defaults defined in the project itself.  See [`reference.conf`](./src/main/resources/reference.conf) for defaults.

+### Reporters


Please add this section to the ToC.

seglo · 2020-02-27T18:49:44Z

README.md

+| Key                         | Default            | Description                                                                                                                           |
+|-----------------------------|--------------------|---------------------------------------------------------------------------------------------------------------------------------------|
+| `reporters.prometheus.port` | `8000`             | The port to run the Prometheus endpoint on                                                                                            |
+| `reporters.graphite.host`   | None               | The graphite host to send metrics to (if not set, will not output to grahpite)                                                        |


seglo · 2020-02-27T18:50:05Z

README.md

+| `reporters.prometheus.port` | `8000`             | The port to run the Prometheus endpoint on                                                                                            |
+| `reporters.graphite.host`   | None               | The graphite host to send metrics to (if not set, will not output to grahpite)                                                        |
+| `reporters.graphite.port`   | None               | The graphite port to send metrics to (if not set, will not output to graphite)                                                        |
+| `reporters.graphite.prefix` | None               | The graphite metric prefix (if not set, prefix will be embty)                                                                         |


seglo · 2020-02-27T18:54:18Z

src/main/scala/com/lightbend/kafkalagexporter/GraphiteEndpointSink.scala

+    }
+  }
+
+  def metricNameToGraphiteMetricName(metricValue: MetricValue): String = {


seglo · 2020-03-06T21:06:31Z

Great, thanks for your patience @yazgoo. I'll push a release soon.

yazgoo · 2020-03-09T08:41:19Z

Thank you !

yazgoo force-pushed the master branch from acce186 to bdff932 Compare January 22, 2020 10:17

yazgoo force-pushed the master branch from bdff932 to df1a4e9 Compare January 22, 2020 10:52

seglo reviewed Jan 26, 2020

View reviewed changes

seglo added enhancement New feature or request refactor Refactor of project labels Jan 28, 2020

yazgoo force-pushed the master branch from 7b824ba to 4892456 Compare January 29, 2020 09:35

✨ add graphite support

fb22f0e

👌 graphite: add periodInSecond conf and stop 🎨 remove graphite bridge 🥅 handle exceptions

yazgoo force-pushed the master branch 7 times, most recently from 6565a3d to d906ed7 Compare January 30, 2020 08:41

yazgoo force-pushed the master branch from d906ed7 to f3002ba Compare February 7, 2020 09:15

🎨 refactoring

0371e33

creating one metric sink dedicated to graphite

yazgoo force-pushed the master branch from f3002ba to 0371e33 Compare February 7, 2020 09:43

seglo reviewed Feb 9, 2020

View reviewed changes

yazgoo force-pushed the master branch 3 times, most recently from 40da0e9 to d4cff9d Compare February 11, 2020 10:04

👌 review fixes

6de7bd9

yazgoo force-pushed the master branch from d4cff9d to 6de7bd9 Compare February 11, 2020 10:13

seglo approved these changes Feb 27, 2020

View reviewed changes

👌 fixes after review

f81aa1a

seglo merged commit 28910ed into seglo:master Mar 6, 2020

seglo mentioned this pull request Mar 6, 2020

Ability to extend and configure desired sink to report lag metrics, adding support to push lag metrics into InfluxDB as well. #130

Closed

seglo mentioned this pull request Sep 1, 2020

Ability to extend and configure custom sink to report lag metrics #129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add graphite support #114

Add graphite support #114

yazgoo commented Jan 22, 2020

lightbend-cla-validator commented Jan 22, 2020

lightbend-cla-validator commented Jan 22, 2020

yazgoo commented Jan 22, 2020

seglo commented Jan 24, 2020

seglo left a comment

yazgoo commented Jan 28, 2020

yazgoo commented Jan 30, 2020

seglo left a comment

seglo Feb 9, 2020

seglo Feb 9, 2020

yazgoo Feb 11, 2020

seglo Feb 27, 2020

seglo Feb 9, 2020

seglo Feb 9, 2020

seglo Feb 9, 2020

seglo Feb 9, 2020

seglo Feb 9, 2020

yazgoo Feb 10, 2020

seglo Feb 9, 2020

yazgoo commented Feb 11, 2020

yazgoo commented Feb 19, 2020

seglo left a comment

seglo Feb 27, 2020

seglo Feb 27, 2020

seglo Feb 27, 2020

seglo Feb 27, 2020

seglo commented Mar 6, 2020

yazgoo commented Mar 9, 2020

		@@ -134,6 +147,7 @@ final case class AppConfig(pollInterval: FiniteDuration, lookupTableSize: Int, p
		\|Prometheus metrics whitelist: [${metricWhitelist.mkString(", ")}]

	\| `reporters.graphite.host` \| None \| The graphite host to send metrics to (if not set, will not output to grahpite) \|
	\| `reporters.graphite.host` \| None \| The graphite host to send metrics to (if not set, will not output to graphite) \|

	\| `reporters.graphite.prefix` \| None \| The graphite metric prefix (if not set, prefix will be embty) \|
	\| `reporters.graphite.prefix` \| None \| The graphite metric prefix (if not set, prefix will be empty) \|

Add graphite support #114

Add graphite support #114

Conversation

yazgoo commented Jan 22, 2020

lightbend-cla-validator commented Jan 22, 2020

lightbend-cla-validator commented Jan 22, 2020

yazgoo commented Jan 22, 2020

seglo commented Jan 24, 2020

seglo left a comment

Choose a reason for hiding this comment

yazgoo commented Jan 28, 2020

yazgoo commented Jan 30, 2020

seglo 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

Choose a reason for hiding this comment

yazgoo commented Feb 11, 2020

yazgoo commented Feb 19, 2020

seglo 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

seglo commented Mar 6, 2020

yazgoo commented Mar 9, 2020