Skip to content
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.

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

Add the endpoint /api/metrics for Prometheus. #175

Merged
merged 14 commits into from
Oct 2, 2017

Conversation

alexeyeryshev
Copy link
Contributor

Add the endpoint /api/metrics for Prometheus.
Delete a logging package.
Small refactoring in Executor.scala.

Copy link
Contributor

@guillaumebort guillaumebort left a comment

Choose a reason for hiding this comment

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

Looks good, just a few scoping issues.

case class Gauge(name: String, value: Long, tags: Seq[(String, String)] = Seq.empty) extends Metric

trait MetricProvider {
private[cuttle] def getMetrics(jobs: Set[String]): Seq[Metric]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't be private[cuttle] if it is exposed as part of the Scheduler API.

@@ -0,0 +1,22 @@
package com.criteo.cuttle
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we scope all this into a Metrics object, so in the public API it's all coherent?

private[cuttle] def getMetrics(jobs: Set[String]): Seq[Metric]
}

object Prometheus {
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be private[cuttle] as it is an implementation detail.

@alexeyeryshev
Copy link
Contributor Author

Corrected

@@ -378,6 +377,32 @@ class Executor[S <: Scheduling] private[cuttle] (
limit: Int): Seq[ExecutionLog] =
queries.getExecutionLog(queryContexts, jobs, sort, asc, offset, limit).transact(xa).unsafePerformIO

private[cuttle] def getStats(jobs: Set[String]): Json = {
val (running, waiting) = runningExecutionsSizes(jobs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I just wanted to point out that provided figures can be inconsistent because stm accesses are non enclosed in a single
transaction. Not use this is critical though

@@ -133,6 +133,10 @@ private[timeseries] object IntervalMap {
m <- this.intersect(interval).toList
} yield m): _*))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You are adding code you do not use, If you really feel this is something that needs to be done I would suggest you could move it to another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned it

@@ -162,4 +166,6 @@ private[timeseries] sealed trait IntervalMap[A, B] {
def mapKeys[K: Ordering](f: A => K): IntervalMap[K, B]
def whenIsDef[C](other: IntervalMap[A, C]): IntervalMap[A, B]
def whenIsUndef[C](other: IntervalMap[A, C]): IntervalMap[A, B]
def head: (Interval[A], B)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@alexeyeryshev
Copy link
Contributor Author

@dufrannea All done

@alexeyeryshev alexeyeryshev merged commit f5d6d8e into criteo:master Oct 2, 2017
@alexeyeryshev alexeyeryshev deleted the eryshev/prometheus_metrics branch October 2, 2017 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants