Add ability to scrape and listen unix domain sockets

[nabokihms]

Signed-off-by: m.nabokikh [email protected]

Proposed changes

Add ability to scrape and listen on unix domain socket.

nginx.conf example for scraping:

http {
  access_log off;

  server {
    server_name _;
    listen unix:/etc/sockets/status.sock;

    location /nginx_status {
      stub_status on;
    }
  }
}

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING guide
• I have proven my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have ensured the README is up to date
• I have rebased my branch onto master
• I will ensure my PR is targeting the master branch and pulling from my branch on my own fork

[pleshakov]

@nabokihms thanks for the PR!

Scraping from a unix domain socket makes sense.

However, could you possibly clarify why it is important for the exporter to bind on a unix domain socket? What would be a use case?

[nabokihms]

@pleshakov thanks for reply!

In my case I run some pod in Kubernetes with nginx, nginx-exporter and some-proxy-for-keep-metrics-protected containers and hostNetwork: true option.

Now I must use one tcp port to expose nginx_stub_status, another one to expose metrics and another one to expose proxy server to make traffic flow works.

But I want to keep all communications behind the scene. We can mount some emptyDir volume to pod, which will contains unix sockets and expose only one tcp port for proxy server.

I want to deploy more than one pod on the same node in the future. Specifying only one port for metrics will be much better than specifying three ports.

In my opinion it's not just Kubernetes use case. It fits for all installations, that wants to keep metrics behind proxy, which locates on the same server.

[pleshakov]

@nabokihms thanks for the explanation! we'll review shortly.

[Rulox]

Hi @nabokihms

First of all thanks very much for the PR and also sorry that it took a while for us to review this.

I have added some comments or suggestions in the PR, let me know your thoughts.

Additionally, I believe it would be nice to update the docs https://github.com/nginxinc/nginx-prometheus-exporter#command-line-arguments especifically nginx.scrape-uri and web.listen-address and web-scrape-uri to make sure the user understands that they can use unix sockets there (and how, eg the format). This change would also involve changing the help string in the flag.String(...) methods in exporter.go as those docs are the same as in the README.md.

Thanks again for your time!

[Rulox]

@nabokihms Thanks!

This looks better now. If @pleshakov approves we'll merge this soon.

Thanks again for your contribution 👍

[pleshakov]

@nabokihms
thanks for the changes! Please see my comments below:

(1)

During additional testing, I discovered a following problem: if you configure the exporter to listen on a unix socket, when the exporter exits, it doesn't remove the unix socket from the file system.

Consider the following example:

$ ./nginx-prometheus-exporter -nginx.scrape-uri=http://demo.nginx.com/stub_status -web.listen-address=unix:./socket
2019/11/07 21:37:16 Starting NGINX Prometheus Exporter Version=0.4.2 GitCommit=56b8d95
2019/11/07 21:37:16 NGINX Prometheus Exporter has successfully started
2019/11/07 21:37:16 Listening on unix:./socket
< here I sent a SIGTERM signal to the exporter process>
2019/11/07 21:37:43 SIGTERM received: terminated. Exiting...

$ ls -l socket
srwxrwxr-x 1 vagrant vagrant 0 Nov  7 21:37 socket

$ ./nginx-prometheus-exporter -nginx.scrape-uri=http://demo.nginx.com/stub_status -web.listen-address=unix:./socket
2019/11/07 21:39:26 Starting NGINX Prometheus Exporter Version=0.4.2 GitCommit=56b8d95
2019/11/07 21:39:26 NGINX Prometheus Exporter has successfully started
2019/11/07 21:39:26 Listening on unix:./socket
2019/11/07 21:39:26 invalid argument

As a result, the second time we start the exporter, it fails to start (the error is somewhat cryptic though). If we remove the socket, the exporter will be able to start again:

$ rm socket

$ ./nginx-prometheus-exporter -nginx.scrape-uri=http://demo.nginx.com/stub_status -web.listen-address=unix:./socket
2019/11/07 21:41:14 Starting NGINX Prometheus Exporter Version=0.4.2 GitCommit=56b8d95
2019/11/07 21:41:14 NGINX Prometheus Exporter has successfully started
2019/11/07 21:41:14 Listening on unix:./socket
. . .

A simple solution would be to make the exporter remove the configured unix socket on start if the socket exits. However, typically, software that creates unix sockets removes them on shutdown. For example, this is true for NGINX. This is considered a best practice.

Do you encounter this problem in your env?

Could you possibly extend the PR to cover the case of the unix socket removal? I suggest we remove on exit/termination.

(2)
When printing error, it is not necessary to use %+v as errors will be printed using their Error() method. For example:

log.Fatalf("parsing unix domain socket listen address %s failed: %+v", listenAddress, err)

Could you possibly remove + from the format strings?

Thanks

[nabokihms]

@pleshakov
(1) I refactored the code to apply Close() method to net.Listener object before exiting on interrupt and sigterm signals. Please, take a look.
(2) Fixed.

[pleshakov]

@nabokihms thanks for the changes! works well!

However, I found a small bug. Missed that during the previous review :(

[pleshakov]

the err in this line is a variable defined in line 137, not the line 134 (var err error). As a result, if net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) returns an error, it will not get caught in line 146:

	if err != nil {
		return listener, err
	}

That's why we got a cryptic message when the exporter tried to bind to the existing unix domain socket:

$ ./nginx-prometheus-exporter -nginx.scrape-uri=http://demo.nginx.com/stub_status -web.listen-address=unix:./socket
2019/11/07 21:39:26 Starting NGINX Prometheus Exporter Version=0.4.2 GitCommit=56b8d95
2019/11/07 21:39:26 NGINX Prometheus Exporter has successfully started
2019/11/07 21:39:26 Listening on unix:./socket
2019/11/07 21:39:26 invalid argument

If you fix the bug, you will get:

2019/11/08 17:11:43 Could not create listener: listen unix socket-exporter: bind: address already in use

[nabokihms]

Yes, I made a mistake here. It's correct now 😉.

[pleshakov]

@nabokihms thanks!!