Add nginx metrics to prometheus #36

aledbf · 2016-11-29T01:41:14Z

bprashanth · 2016-11-29T01:48:22Z

controllers/nginx/pkg/cmd/controller/metrics.go

+}
+
+func (n *NGINXController) setupMonitor(args []string) {
+	pc, err := newProcessCollector(true, exeMatcher{"nginx", args})


why run this in the nginx controller vs in the generic controller and report Status through an interface method? prometheus library seems pretty general purpose, and we could just ask the backend to return a map of like:

map [string]string{ "num_procs": 10, "read_bytes": 123, ... }

Which the generic_controller converts into some export format (could be prometheus, could be write directly to some database etc)

because the generic_controller already exposes prometheus metrics for the go process. Using this approach each backend can decide what to export and the comments for each variable (and how to extract the information)

(could be prometheus, could be write directly to some database etc)

can we take this as an improvement and iterate?

bprashanth

would be great if we could avoid the duplication for each metric, otherwise this mostly lgtm after nits are fixed

bprashanth · 2016-11-29T02:19:20Z

controllers/nginx/pkg/cmd/controller/metrics.go

+		glog.Warningf("unexpected error obtaining nginx status info: %v", err)
+		return
+	}
+


there's a lot of repetition here, though I haven't spent time thinking about a better solution

this should be isolated to this backend.
In case of caddy or haproxy there's already an exporter:
https://github.com/miekg/caddy-prometheus
https://github.com/prometheus/haproxy_exporter

bprashanth · 2016-11-29T02:22:29Z

controllers/nginx/pkg/cmd/controller/nginx.go

+	n.start(cmd, done)
+	select {
+	case err := <-done:
+		if exitError, ok := err.(*exec.ExitError); ok {


please explain why this is needed (i.e why is it important to wait for master process in this way vs just running start and assuming the liveness check will fail if the master never comes up?)

I will add the comment in the code. Basically if the nginx master process dies the workers continue to process requests (passing the checks) but in case of updates in ingress no updates will be reflected in the nginx configuration

please explain why this is needed

done

bprashanth · 2016-11-29T02:24:23Z

controllers/nginx/pkg/cmd/controller/status.go

+}
+
+func getNginxStatus() (*nginxStatus, error) {
+	resp, err := http.DefaultClient.Get("http://localhost:18080/internal_nginx_status")


please make this port a const, and the url path

coveralls · 2016-11-29T19:29:14Z

Coverage decreased (-1.0%) to 39.239% when pulling a43d2b1e66db64f9a4a21820b4ae144626155c17 on aledbf:prometheus-nginx into 666cbf5 on kubernetes:master.

k8s-oncall · 2016-11-29T19:43:26Z

This change is

coveralls · 2016-11-29T21:27:28Z

Coverage decreased (-1.02%) to 39.178% when pulling f7011d2 on aledbf:prometheus-nginx into 666cbf5 on kubernetes:master.

aledbf · 2016-11-30T16:41:47Z

@bprashanth ping

bprashanth

LGTM, the simplest way to handle zombies is probably the shell but that can come as a follow up if testing turns out ok

bprashanth · 2016-11-30T17:40:08Z

controllers/nginx/rootfs/Dockerfile

@@ -23,4 +23,10 @@ RUN DEBIAN_FRONTEND=noninteractive apt-get update && apt-get install -y \

 COPY . /

+# https://blog.phusion.nl/2015/01/20/docker-and-the-pid-1-zombie-reaping-problem


can we just rely on the shell to manage zombies?
eg http://blog.dscpl.com.au/2015/12/issues-with-running-as-pid-1-in-docker.html

* use go mod * update travis to use go 1.13 * fix copyright check * fix UT error

aledbf added the enhancement label Nov 29, 2016

aledbf self-assigned this Nov 29, 2016

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 29, 2016

bprashanth reviewed Nov 29, 2016

View reviewed changes

Restart nginx if master process dies

81cd778

aledbf force-pushed the prometheus-nginx branch 3 times, most recently from e19c4ff to a43d2b1 Compare November 29, 2016 19:12

aledbf changed the title ~~WIP: Add nginx metrics to prometheus~~ Add nginx metrics to prometheus Nov 29, 2016

aledbf added 2 commits November 29, 2016 18:10

Add nginx metrics to prometheus

86dbf97

Update godeps

f7011d2

aledbf force-pushed the prometheus-nginx branch from a43d2b1 to f7011d2 Compare November 29, 2016 21:10

bprashanth reviewed Nov 30, 2016

View reviewed changes

bprashanth added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2016

bprashanth merged commit ac6930b into kubernetes:master Nov 30, 2016

aledbf deleted the prometheus-nginx branch December 21, 2016 00:15

aledbf mentioned this pull request Dec 21, 2016

Nginx vts as prometheus metrics #72

Closed

bprashanth mentioned this pull request Feb 6, 2017

Kubernetes components should export metrics in Prometheus format kubernetes/kubernetes#40736

Closed

15 tasks

auhlig mentioned this pull request Mar 3, 2017

update ingress controller to 0.9.0-beta.2 and add prometheus scrape annotations to service sapcc/helm-charts#64

Merged

joushx mentioned this pull request Oct 29, 2018

Affinity cookie not updated if invalid cookie is sent #3317

Closed

haoqing0110 referenced this pull request in stolostron/management-ingress Mar 5, 2021

use go mod (#35) (#36)

ca390cd

* use go mod * update travis to use go 1.13 * fix copyright check * fix UT error

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nginx metrics to prometheus #36

Add nginx metrics to prometheus #36

aledbf commented Nov 29, 2016

bprashanth Nov 29, 2016

aledbf Nov 29, 2016

aledbf Nov 29, 2016

bprashanth left a comment

bprashanth Nov 29, 2016

aledbf Nov 29, 2016

bprashanth Nov 29, 2016

aledbf Nov 29, 2016

aledbf Nov 29, 2016

bprashanth Nov 29, 2016

aledbf Nov 29, 2016

coveralls commented Nov 29, 2016

k8s-oncall commented Nov 29, 2016

coveralls commented Nov 29, 2016

aledbf commented Nov 30, 2016

bprashanth left a comment

bprashanth Nov 30, 2016

		@@ -23,4 +23,10 @@ RUN DEBIAN_FRONTEND=noninteractive apt-get update && apt-get install -y \

		COPY . /

		# https://blog.phusion.nl/2015/01/20/docker-and-the-pid-1-zombie-reaping-problem

Add nginx metrics to prometheus #36

Add nginx metrics to prometheus #36

Conversation

aledbf commented Nov 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bprashanth 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

coveralls commented Nov 29, 2016

k8s-oncall commented Nov 29, 2016

coveralls commented Nov 29, 2016

aledbf commented Nov 30, 2016

bprashanth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment