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

Add client ip to http access logs #1448

Merged
merged 6 commits into from
Jan 5, 2016

Conversation

gozer
Copy link
Contributor

@gozer gozer commented Nov 24, 2015

Fixes #1447

@slackpad
Copy link
Contributor

Hi @gozer - would you mind sticking the address at the end of the log message and using the new log functions from memberlist, so these will all look the same? Here's an example: https://github.com/hashicorp/consul/blob/master/consul/rpc.go#L86. There's a LogAddr function that should work for your use case here. Thanks!

@gozer
Copy link
Contributor Author

gozer commented Dec 21, 2015

Absolutely, will do and update my PR

@gozer
Copy link
Contributor Author

gozer commented Dec 22, 2015

memberlist.LogAddress wants a net.Addr, and req.RemoteAddr is already just a string, not an address anymore, so not sure it's worth trying to convert it back to a net.Addr

@slackpad
Copy link
Contributor

@gozer sorry I didn't realize that. In that case, it should be good enough to stick it at the end as "... from=%s" to match the memberlist format. Agree it's not worth converting to an address.

@gozer
Copy link
Contributor Author

gozer commented Dec 30, 2015

@slackpad done as suggested

@slackpad
Copy link
Contributor

slackpad commented Jan 5, 2016

@gozer thanks for the update! One other thing - I think your split() call will mangle IPV6 addresses - since it's just a string I'd probably just print it with the port number.

@gozer
Copy link
Contributor Author

gozer commented Jan 5, 2016

@slackpad, you are absolutely correct, that address splitting had bothered me from the start, to be honest. Removed as suggested.

slackpad added a commit that referenced this pull request Jan 5, 2016
@slackpad slackpad merged commit ae7e96a into hashicorp:master Jan 5, 2016
@slackpad
Copy link
Contributor

slackpad commented Jan 5, 2016

LGTM - Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants