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

Support log_format combined setting of NGINX access logs #6858

Merged

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Apr 13, 2018

The following log line was not parsed successfully:

127.0.0.1 - - [12/Apr/2018:09:48:40 +0200] "" 400 0 "-" "-"

"" had to match the pattern \"%{WORD:nginx.access.method} %{DATA:nginx.access.url} HTTP/%{NUMBER:nginx.access.http_version}\"

It failed because those fields had to be there, empty values were not accepted. Also the pattern looks for HTTP/ which was also missing.

Now missing fields are allowed in this part of the message.

Closes #6798

@ph
Copy link
Contributor

ph commented Apr 13, 2018

@kvch can you add a test case for that change?

@kvch kvch force-pushed the fix/filebeat/fix/nginx-access-combined-log-format branch from aa5fff1 to 2b13f13 Compare April 13, 2018 15:01
@kvch
Copy link
Contributor Author

kvch commented Apr 13, 2018

sure.
something is still failing due to my change.

@kvch
Copy link
Contributor Author

kvch commented Apr 13, 2018

Should be fixed now. Let's see what the CIs have to say. :)

@kvch kvch force-pushed the fix/filebeat/fix/nginx-access-combined-log-format branch from a3b0c78 to 10aec0a Compare April 16, 2018 12:36
@kvch kvch force-pushed the fix/filebeat/fix/nginx-access-combined-log-format branch from 10aec0a to 670ed57 Compare April 16, 2018 13:43
@ph
Copy link
Contributor

ph commented Apr 16, 2018

@kvch Changes look good, waiting for Travis and Jenkins to do the CI dance.

@kvch
Copy link
Contributor Author

kvch commented Apr 16, 2018

@ph The failing test is unrelated to this change.

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ph ph merged commit 13fca49 into elastic:master Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants