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

Improve logging #1659

Merged
merged 2 commits into from
Jun 14, 2021
Merged

Improve logging #1659

merged 2 commits into from
Jun 14, 2021

Conversation

exussum12
Copy link
Contributor

Currently its almost impossible to know which host actually handled the request, this extra variable logs the upstream server too

Currently its almost impossible to know which host actually handled the request, this extra variable logs the upstream server too
@buchdag
Copy link
Member

buchdag commented Jun 10, 2021

Hi ! Thanks for the PR !

Do you have a before PR / after PR example of the log ?

@exussum12
Copy link
Contributor Author

exussum12 commented Jun 11, 2021

Sure.
Easiest to set up scenario is

Terminal 1
docker run -p 80:80 -v /var/run/docker.sock:/tmp/docker.sock:ro jwilder/nginx-proxy

Terminal 2
docker run -e VIRTUAL_HOST=test.local nginx:latest

Terminal 3
docker run -e VIRTUAL_HOST=test.local nginx:latest

The proxy will load balance between the two
Terminal 4

curl 127.0.0.1 -H "Host: test.local"
curl 127.0.0.1 -H "Host: test.local" 

One request is served by each and the resulting logs are

nginx.1    | test.local 172.17.0.1 - - [11/Jun/2021:10:09:37 +0000] "GET / HTTP/1.1" 200 612 "-" "curl/7.74.0"
nginx.1    | test.local 172.17.0.1 - - [11/Jun/2021:10:09:41 +0000] "GET / HTTP/1.1" 200 612 "-" "curl/7.74.0"

After this change

nginx.1    | test.local 172.17.0.1 - - [11/Jun/2021:10:14:41 +0000] "GET / HTTP/1.1" 200 612 "-" "curl/7.74.0" "172.17.0.4:80"
nginx.1    | test.local 172.17.0.1 - - [11/Jun/2021:10:14:42 +0000] "GET / HTTP/1.1" 200 612 "-" "curl/7.74.0" "172.17.0.3:80"

The last param makes it easier work see where the request ended up.

I added to the end to prevent breaking any existing awk scripts which people have have written

@buchdag
Copy link
Member

buchdag commented Jun 14, 2021

Seems okay to me, I have no objection to merging this but I'm a bit concerned by people relying on load balancing with nginx-proxy. This was never intended as a feature, is untested and kinda just work "by accident".

@buchdag buchdag added the type/feat PR for a new feature label Jun 14, 2021
@exussum12
Copy link
Contributor Author

Its not just load balancing it helps with, another use case is confirming you are going to the correct container in a default fall back. Eg nginx doesn't match as expected, and serves the default vhost instead.

The load balancing case just makes it clear that the number changes.

@buchdag buchdag merged commit eb00b06 into nginx-proxy:main Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feat PR for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants