-
Notifications
You must be signed in to change notification settings - Fork 26
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 prometheus endpoint and metrics #45
Conversation
@@ -1 +1,3 @@ | |||
slow_cooker* | |||
Godeps/ | |||
vendor/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone back and forth on this, but my preference is to check both of these directories into source control. Mind reverting the change in this file and adding Godeps/ and vendor/ to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cool! In the future it could be nice to parameterize main
in a way that allows users to conditionally use different stat types ( a la linkerd plugin system, but for go interfaces :)
@@ -198,6 +227,7 @@ func main() { | |||
headers := make(headerSet) | |||
flag.Var(&headers, "header", "HTTP request header. (can be repeated.)") | |||
data := flag.String("data", "", "HTTP request data") | |||
addr := flag.String("addr", ":8505", "service port to run on") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote to call this flag "metric-addr" or something more specific, since it only pertains the prometheus endpoint. I wouldn't want somebody confusing this with the addr of the host to which we're sending traffic. Also, I'd prefer for this flag to default to "", and for us not to start the metrics server at all, unless the addr is explicitly provided via the flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐️ Thanks for making those updates -- looks good to me.
this change introduces a
/metrics
endpoint to the slow_cooker process.it currently exports 3 stats
note that unlike the existing histogram code, it only records latencies for successful requests. the intent is to not report low latencies to services that are failing fast. open to discussion on this.