Skip to content

accesslog: Append a newline to returned log_lines in access_log#5755

Closed
cmluciano wants to merge 5 commits intoenvoyproxy:masterfrom
cmluciano:cml/stringaccesslog
Closed

accesslog: Append a newline to returned log_lines in access_log#5755
cmluciano wants to merge 5 commits intoenvoyproxy:masterfrom
cmluciano:cml/stringaccesslog

Conversation

@cmluciano
Copy link
Copy Markdown
Member

@cmluciano cmluciano commented Jan 29, 2019

Signed-off-by: Christopher M. Luciano cmluciano@us.ibm.com

Description: Append a newline to returned log_line in access_log
Risk Level: Medium - Might be worked around by external consumers already that add a newline themselves
Testing: Adjusted tests to account for newline
Docs Changes: TODO
Release Notes: TODO
Fixes #5193

Notes for reviewer:

  • This change has the unfortunate side-effect of breaking the verifyJsonOutput test since it expects only one ending terminator and does not expect the first pair to contain a newline.
  • Is it sufficient to drop some of these expects and update the strings in the JSON tests or have I gone about adding this the wrong way?

accesslog: Append a newline to returned log_lines in access_log
677a16e 
Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
@ggreenway ggreenway self-assigned this Jan 29, 2019
ggreenway
ggreenway requested changes Jan 29, 2019
View reviewed changes
Christopher M. Luciano added 2 commits January 30, 2019 11:24
access_log: append newline via PlainStringFormatter if missing
40ca78c 
Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
access_log: only add newline if it doesn't exist
2d37986 
Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
@cmluciano
Copy link
Copy Markdown
Member Author

cmluciano commented Jan 30, 2019

@ggreenway Is it still expected that I should have to update the json stubs to expect a newline for the first.pair?

@GregHanson GregHanson mentioned this pull request Jan 31, 2019
Merged
@ggreenway
Copy link
Copy Markdown
Member

ggreenway commented Jan 31, 2019

I haven't looked closely at how json logs are output. I thought they were using the protobuf-->json formatter; I'm unsure if they share code with the "normal" text access logger. So I don't know the answer to your question.

access_log: update tests for parser changes
3221288 
Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
ggreenway
ggreenway requested changes Feb 1, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Oh, now I understand your previous question. It looks like with this change as it is now, a newline is being inserted into each json value, which is not correct. A newline at the end of the json object comprising a single log-entry is what we want (and I think this already works for json logging).

The same call to parse() is being made from here:

envoy/source/common/access_log/access_log_formatter.cc

Line 76 in 5a88c95

auto providers = AccessLogFormatParser::parse(pair.second);
, once per json value. You'll need to somehow change this so that the auto-newline is only added to the plain-text format, not to json. Possibly add a param to parse(), or figure out another way to do that conditionally.

access_log: fix formatter test format
17e9b12 
Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>
@stale
Copy link
Copy Markdown

stale bot commented Feb 11, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 11, 2019
@cmluciano
Copy link
Copy Markdown
Member Author

cmluciano commented Feb 12, 2019

not stale, just working on other PRs first

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 12, 2019
@stale
Copy link
Copy Markdown

stale bot commented Feb 19, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 19, 2019
@stale
Copy link
Copy Markdown

stale bot commented Feb 26, 2019

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Feb 26, 2019
@drmaciej drmaciej mentioned this pull request Nov 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggreenway ggreenway ggreenway requested changes

Assignees

@ggreenway ggreenway

Labels

stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@cmluciano @ggreenway