Skip to content

Implement omit_empty_values for access log formatters#12801

Merged
dio merged 15 commits intoenvoyproxy:masterfrom
Pchelolo:json_formatter_skip_nulls
Sep 4, 2020
Merged

Implement omit_empty_values for access log formatters#12801
dio merged 15 commits intoenvoyproxy:masterfrom
Pchelolo:json_formatter_skip_nulls

Conversation

@Pchelolo
Copy link
Copy Markdown
Contributor

@Pchelolo Pchelolo commented Aug 24, 2020

Signed-off-by: Petr Pchelko ppchelko@wikimedia.org

Commit Message: Introduce an option to entirely omit null values from access log
Additional Description:
Risk Level: Low
Testing: unit/integration tests
Docs Changes: new option documented in proto file
Release Notes: updated
Fixes #12735

Implement omit_empty_values for access log formatters
53da9e0 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Aug 24, 2020

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #12801 was opened by Pchelolo.

see: more, trace.

Have a default value for FormatterImpl constructor
4cb6bb2 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
zuercher
zuercher reviewed Aug 25, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

What do you think of changing this around so that Formatter::format returns absl::optional<std::string>? That makes it so that we don't have to re-implement the return empty string vs "-" logic in each formatter and can make the decision in FormatterImpl.

@Pchelolo
Copy link
Copy Markdown
Contributor Author

Pchelolo commented Aug 25, 2020

What do you think of changing this around so that Formatter::format returns absl::optional<std::string>? That makes it so that we don't have to re-implement the return empty string vs "-" logic in each formatter and can make the decision in FormatterImpl.

Yeah... When I've started I didn't realize how many places would need updating, changing the return type would prob be better.

Make FormatterProvider return optional
f1d6440 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
@Pchelolo
Copy link
Copy Markdown
Contributor Author

Pchelolo commented Aug 26, 2020

Changed the FormatterProvider interface to return an optional since it's reponsible for fetching a single value from the operator. Formatter::format returns a combination of multiple providers, so it doesn't really make sense to return an optional from it.

Petr Pchelko added 3 commits August 25, 2020 20:57
Revert some unnesessary formatting changes
0ac29ee 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
Fix format
e99fec7 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
Added some more tests and utility functions
f3e0d5e 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
zuercher
zuercher reviewed Aug 26, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks! I think this will be better in the long run. Some mostly style related stuff around use of optional but otherwise looks good.

Petr Pchelko added 2 commits August 26, 2020 11:14
Use of optional.value_or and other review comment fixes
a8697b6 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
Fix format
c2e12ac 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
@Pchelolo
Copy link
Copy Markdown
Contributor Author

Pchelolo commented Aug 26, 2020

Addressed review comments, thank you @zuercher. Also added a few more tests and a few convenience utility functions.

Merge branch 'master' into json_formatter_skip_nulls
71c9434 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
@Pchelolo Pchelolo requested a review from zuercher August 26, 2020 23:19
zuercher
zuercher reviewed Aug 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks! Can you run the substitution_formatter_speed_test before/after and make sure there's no performance regression?

@zuercher
Copy link
Copy Markdown
Member

zuercher commented Aug 27, 2020

Also needs @envoyproxy/api-shepherds approval.

@Pchelolo
Copy link
Copy Markdown
Contributor Author

Pchelolo commented Aug 27, 2020

Thanks! Can you run the substitution_formatter_speed_test before/after and make sure there's no performance regression?

Thank you @zuercher

Master:

-------------------------------------------------------------------------
Benchmark                               Time             CPU   Iterations
-------------------------------------------------------------------------
BM_AccessLogFormatter                6571 ns         6512 ns       104581
BM_JsonAccessLogFormatter          182054 ns       180735 ns         3704
BM_TypedJsonAccessLogFormatter     175966 ns       174728 ns         4020

Feature branch:

-------------------------------------------------------------------------
Benchmark                               Time             CPU   Iterations
-------------------------------------------------------------------------
BM_AccessLogFormatter                7268 ns         7234 ns        87323
BM_JsonAccessLogFormatter          173080 ns       172052 ns         4065
BM_TypedJsonAccessLogFormatter     170383 ns       168708 ns         4009

@Pchelolo Pchelolo requested a review from zuercher August 27, 2020 19:20
zuercher
zuercher reviewed Aug 28, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Seems to be about 10% slower, which is somewhat concerning. Are those results stable across runs?

My previous experience with this code tells me that the perf changes are normally due to extra string copies, but it doesn't seem like this introduces any except for protocol where before we were able to return a const std::string& from the utility class, but is now copying the string into an absl::optional<std::string>.

Actually, I wonder if value_or is the problem. It returns T, not T& so maybe that's actually inducing a string copy. Assuming the slowdown wasn't a side-effect of something else happening on your dev box, could you experiment with not using value_or? It's definitely easier to read with that syntax but I'd rather not slow the loggers down. If you do end up removing value_or please add a comment explaining why.

Petr Pchelko added 2 commits August 28, 2020 14:03
Address review comments
a27640d 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
Made protocolToString return an optional reference
13adabe 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
@Pchelolo
Copy link
Copy Markdown
Contributor Author

Pchelolo commented Aug 28, 2020

Here's the new benchmarks: https://gist.github.com/Pchelolo/704546cde3cf9fe9867eae00cfad8a9d

The value_or does not make any difference at all, but making the protocolToString return an optional reference wrapper seem to have made the new version slightly faster, so perf degradation is about 6% now.

@Pchelolo Pchelolo requested a review from zuercher August 28, 2020 22:56
Petr Pchelko added 2 commits August 28, 2020 16:16
Fix tests
3335f91 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
Fix format
d53c260 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
@htuch
Copy link
Copy Markdown
Member

htuch commented Aug 31, 2020

/lgtm api

zuercher
zuercher reviewed Sep 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks! One last thing and I think we're good to go.

source/common/formatter/substitution_formatter.cc
}

const absl::optional<std::string>
const absl::optional<std::reference_wrapper<const std::string>>
Copy link
Copy Markdown
Member

@zuercher zuercher Sep 1, 2020

Choose a reason for hiding this comment

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

Let's add a comment here explaining why this is a reference_wrapper.

Copy link
Copy Markdown
Contributor Author

@Pchelolo Pchelolo Sep 1, 2020

Choose a reason for hiding this comment

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

Done!

Added a comment about reference_wrapper
a7fb244 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
@Pchelolo Pchelolo requested a review from zuercher September 1, 2020 18:30
zuercher
zuercher reviewed Sep 1, 2020
View reviewed changes
source/common/formatter/substitution_formatter.h Outdated
class SubstitutionFormatUtils {
public:
static FormatterPtr defaultSubstitutionFormatter();
// Optional refences are not supported, but this method has large performance
Copy link
Copy Markdown
Member

@zuercher zuercher Sep 1, 2020

Choose a reason for hiding this comment

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

*references

Copy link
Copy Markdown
Contributor Author

@Pchelolo Pchelolo Sep 1, 2020

Choose a reason for hiding this comment

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

Omg, sorry about that. Fixed.

Fix typo
cb3efbe 
Signed-off-by: Petr Pchelko <ppchelko@wikimedia.org>
@Pchelolo Pchelolo requested a review from zuercher September 1, 2020 21:20
zuercher
zuercher approved these changes Sep 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

Thanks!

@dio dio merged commit 9d466c7 into envoyproxy:master Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuercher zuercher zuercher approved these changes

+2 more reviewers

@htuch htuch htuch left review comments

@dio dio dio approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@zuercher zuercher

@dio dio

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Proposal: introduce an option to skip JSON keys with null values in json log format

4 participants

@Pchelolo @zuercher @htuch @dio