-
Notifications
You must be signed in to change notification settings - Fork 619
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
access logging #80
Comments
Not yet but certainly doable. Just log to local disk? |
thx for the quick reply! |
so access log to stdout, normal log to stderr? |
That would be perfect for me |
Any preference on the format? |
No strong preference really, but json would be easy to parse. The data I capture in other places would be
Of course for a first version it would be really nice just having the first two! Let me know if there is anything I could do to help with this. :) |
I guess, the format should be configurable, say we'd like to have some headers as well. Here is implementation from logback, might be useful: http://logback.qos.ch/access.html |
Original PR #131 by [email protected] This patch adds support for configurable access logging which is compatible with nginx config parameters.
* Add benchmark and numbers * Add more tests * Do not alloc
What is still missing to be able to merge this? |
There was quite a bit of work to do AFAIR. I'm on vacation for another week and should have time to pick some of the slack early December. |
Thanks a lot for your fast feedback Frank, |
+1 for access logs, also to a file too |
Also wanted to add that access logging would be super awesome. |
I'm not sure I see the rationale to have this goto a file natively. Running fabio under systemd, upstart or in a docker container allows you to easily get any stdout/stderr into a file with standard tools. |
Lets first get this working. Writing to a file is trivial but I agree with @leprechau that this shouldn't be necessary in this day and age. |
This patch is based on the PR #131 from [email protected]. It adds support for configurable access logging to either stdout or stderr for HTTP requests. Fixes #80
This patch is based on the PR #131 from [email protected]. It adds support for configurable access logging to either stdout or stderr for HTTP requests. Fixes #80
OK, here is access logging support for fabio. The code supports the common and combined log file format out of the box with You can also configure a custom log format. Check I've spent some time writing the code in a way so that it doesn't alloc since fabio sits in the hot-path. For everybody interested in this feature: Testing is highly appreciated. I'm especially interested in whether the values are correct and whether all the fields you're looking for are covered. This branch is based on |
This patch is based on the PR #131 from [email protected]. It adds support for configurable access logging to either stdout or stderr for HTTP requests. Fixes #80
Rebased the patch on the new 1.4 release which is on master now. |
This patch is based on the PR #131 from [email protected]. It adds support for configurable access logging to either stdout or stderr for HTTP requests. Fixes #80
This patch is based on the PR #131 from [email protected]. It adds support for configurable access logging to either stdout or stderr for HTTP requests. Fixes #80
This patch is based on the PR #131 from [email protected]. It adds support for configurable access logging to either stdout or stderr for HTTP requests. Fixes #80
Rebased to master after travis test failure fix. |
I can confirm that |
Cool and thanks @holtwilkins for taking another look. @sielaq has also found the nil pointer issue so I think the code is in good shape. I'll sit on this for a couple more days since I don't like the config options yet because they clash a bit with the |
$upstream_host logs protocol pre-pended to the host, $upstream_port leaves slash after the port:
|
@ak66982 No, of course not. Should have written a full integration test :( I'll fix it. |
This patch is based on the PR #131 from [email protected]. It adds support for configurable access logging to either stdout or stderr for HTTP requests. Fixes #80
@ak66982 Fixed. |
This patch is based on the PR #131 from [email protected]. It adds support for configurable access logging to either stdout or stderr for HTTP requests. Fixes #80
This produces now: I've added a couple of additional fields:
The full list is:
|
This patch is based on the PR #131 from [email protected]. It adds support for configurable access logging to either stdout or stderr for HTTP requests. Fixes #80
By the way, thanks for the wonderful piece of software!
I checked the tcpdump, there was nothing strange in these requests. |
@ak66982 Thank you. I'm glad you like it. Right now, I'm getting the content length from the http response generated by the Go reverse proxy. i didn't dig into how it retrieves it but I'll have a look. |
* Add integration test * Fix upstream_uri and upstream_url values
* Add integration test * Fix upstream_uri and upstream_url values * Fix request_url value
Added a full integration tests which generates the log output through the proxy with a real request. This uncovered two bugs in the The other thing I've discovered was that the request object got modified through the reverse proxy code which made the @ak66982 I still don't have a test for the |
Move log config parameters to global level: - log.access.format - log.access.target - log.routes.format
I've renamed the log parameters as follows:
This should also make integration with PR #246 more consistent. If there are no objections then I'm going to merge this and release version 1.4.1. |
Fix tests, add comments and make pattern code more robust.
More polishing Make sure all metrics log the same duration
Published release v1.4.1 with access logging support. Thanks for your help. |
Hi!
First off, thank you for making this, it's a really cool project!
My question is quite simple, does fabio support access logging? nginx for instance can print all requests passing through and their responses to stdout or a file. Would be quite useful to me, since I'd then would not need to implement this in all downstream services.
The text was updated successfully, but these errors were encountered: