-
Notifications
You must be signed in to change notification settings - Fork 92
Enable statsd metrics collection for histograms #91
Conversation
@stevesloka Is this something that we want to merge with the unofficial image? Or will we wait to get the contour PR merged before merging this? |
# metrics. This is because Envoy does not expose prometheus metrics | ||
# without setting the format=prometheus URL parameter on Envoy's stats | ||
# enpdoint. | ||
- source_labels: [__meta_kubernetes_pod_annotation_prometheus_io_format] |
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 think we should be able to get rid of this annotation for statsd.
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.
We can't, we need to scrape statsd on a different port + mapping. This one reads a different annotation than the normal one.
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.
We need the prometheus.io/format
annotation for statsd as well?
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.
Ahh I see, sorry misread your comment. Yeah I can ditch that one, it's redundant.
kubeconfig
Outdated
@@ -0,0 +1,33 @@ | |||
apiVersion: v1 |
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 file slipped through?
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.
Yeah 😞
@alexbrand I removed that config from the prometheus config, also I took out the |
@stevesloka Sweet! Are we OK with merging this with an unofficial image? |
@stevesloka and I talked out of band. We decided to merge this using the official contour image with the understanding that statsd metrics will work once the Contour PR is merged. |
Was about to merge but then realized that the |
Contour has merged the statsd PR, so this is now unblocked. I updated the deployment to use the |
Let me review a couple of the stats quickly, might of found an issue. |
Signed-off-by: Steve Sloka <steves@heptio.com>
I did some testing, the metrics work fine, however, |
Signed-off-by: Steve Sloka <steves@heptio.com>
This PR is ready to go, Contour is now fixed on master and I've updated the deployments to use that tag. |
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.
LGTM!
This enables histogram support for Envoy metrics by using the statsd exporter to collect data from Envoy and then allow Prometheus to scrape for those metrics.
Note this PR has non-standard Contour builds as it depends on changes to enable statsd endpoints. PR for that is projectcontour/contour#366. Once that merges we can update the image to use a
master
tag until a new release of Contour.Fixes #92
Signed-off-by: Steve Sloka steves@heptio.com