Implement omit_empty_values for access log formatters #12801

Pchelolo · 2020-08-24T22:39:02Z

Signed-off-by: Petr Pchelko [email protected]

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

Signed-off-by: Petr Pchelko <[email protected]>

repokitteh-read-only · 2020-08-24T22:39:08Z

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.

Signed-off-by: Petr Pchelko <[email protected]>

api/envoy/config/core/v3/substitution_format_string.proto

zuercher

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 · 2020-08-25T22:04:11Z

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.

Signed-off-by: Petr Pchelko <[email protected]>

Pchelolo · 2020-08-26T03:21:44Z

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.

Signed-off-by: Petr Pchelko <[email protected]>

zuercher

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

source/common/formatter/substitution_formatter.cc

source/common/protobuf/utility.cc

source/common/router/header_formatter.cc

source/common/tracing/http_tracer_impl.cc

Signed-off-by: Petr Pchelko <[email protected]>

Pchelolo · 2020-08-26T18:31:48Z

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

Signed-off-by: Petr Pchelko <[email protected]>

zuercher

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

zuercher · 2020-08-27T18:40:50Z

Also needs @envoyproxy/api-shepherds approval.

Pchelolo · 2020-08-27T19:02:00Z

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

zuercher

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.

source/common/formatter/substitution_formatter.cc

source/common/router/header_formatter.cc

Signed-off-by: Petr Pchelko <[email protected]>

Pchelolo · 2020-08-28T22:56:44Z

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.

Signed-off-by: Petr Pchelko <[email protected]>

htuch · 2020-08-31T12:54:48Z

/lgtm api

zuercher

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

zuercher · 2020-09-01T15:26:25Z

source/common/formatter/substitution_formatter.cc

@@ -65,15 +65,15 @@ FormatterPtr SubstitutionFormatUtils::defaultSubstitutionFormatter() {
  return FormatterPtr{new FormatterImpl(DEFAULT_FORMAT, false)};
 }

-const absl::optional<std::string>
+const absl::optional<std::reference_wrapper<const std::string>>


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

Signed-off-by: Petr Pchelko <[email protected]>

zuercher · 2020-09-01T20:39:21Z

source/common/formatter/substitution_formatter.h

@@ -73,6 +73,8 @@ class SubstitutionFormatParser {
 class SubstitutionFormatUtils {
 public:
  static FormatterPtr defaultSubstitutionFormatter();
+  // Optional refences are not supported, but this method has large performance


*references

Omg, sorry about that. Fixed.

Signed-off-by: Petr Pchelko <[email protected]>

zuercher

Thanks!

Implement omit_empty_values for access log formatters

53da9e0

Signed-off-by: Petr Pchelko <[email protected]>

repokitteh-read-only bot added the api label Aug 24, 2020

Have a default value for FormatterImpl constructor

4cb6bb2

Signed-off-by: Petr Pchelko <[email protected]>

mattklein123 assigned zuercher Aug 24, 2020

htuch reviewed Aug 25, 2020

View reviewed changes

api/envoy/config/core/v3/substitution_format_string.proto Show resolved Hide resolved

zuercher reviewed Aug 25, 2020

View reviewed changes

Make FormatterProvider return optional

f1d6440

Signed-off-by: Petr Pchelko <[email protected]>

Petr Pchelko added 3 commits August 25, 2020 20:57

Revert some unnesessary formatting changes

0ac29ee

Signed-off-by: Petr Pchelko <[email protected]>

Fix format

e99fec7

Signed-off-by: Petr Pchelko <[email protected]>

Added some more tests and utility functions

f3e0d5e

Signed-off-by: Petr Pchelko <[email protected]>

zuercher reviewed Aug 26, 2020

View reviewed changes

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 <[email protected]>

Fix format

c2e12ac

Signed-off-by: Petr Pchelko <[email protected]>

Merge branch 'master' into json_formatter_skip_nulls

71c9434

Signed-off-by: Petr Pchelko <[email protected]>

Pchelolo requested a review from zuercher August 26, 2020 23:19

zuercher reviewed Aug 27, 2020

View reviewed changes

Pchelolo requested a review from zuercher August 27, 2020 19:20

zuercher reviewed Aug 28, 2020

View reviewed changes

source/common/formatter/substitution_formatter.cc Show resolved Hide resolved

source/common/formatter/substitution_formatter.cc Outdated Show resolved Hide resolved

source/common/router/header_formatter.cc Outdated Show resolved Hide resolved

Petr Pchelko added 2 commits August 28, 2020 14:03

Address review comments

a27640d

Signed-off-by: Petr Pchelko <[email protected]>

Made protocolToString return an optional reference

13adabe

Signed-off-by: Petr Pchelko <[email protected]>

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 <[email protected]>

Fix format

d53c260

Signed-off-by: Petr Pchelko <[email protected]>

repokitteh-read-only bot removed the api label Aug 31, 2020

zuercher reviewed Sep 1, 2020

View reviewed changes

Added a comment about reference_wrapper

a7fb244

Signed-off-by: Petr Pchelko <[email protected]>

Pchelolo requested a review from zuercher September 1, 2020 18:30

zuercher reviewed Sep 1, 2020

View reviewed changes

Fix typo

cb3efbe

Signed-off-by: Petr Pchelko <[email protected]>

Pchelolo requested a review from zuercher September 1, 2020 21:20

zuercher approved these changes Sep 2, 2020

View reviewed changes

zuercher assigned dio Sep 2, 2020

dio approved these changes Sep 4, 2020

View reviewed changes

dio merged commit 9d466c7 into envoyproxy:master Sep 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement omit_empty_values for access log formatters #12801

Implement omit_empty_values for access log formatters #12801

Pchelolo commented Aug 24, 2020

repokitteh-read-only bot commented Aug 24, 2020

zuercher left a comment

Pchelolo commented Aug 25, 2020

Pchelolo commented Aug 26, 2020

zuercher left a comment

Pchelolo commented Aug 26, 2020

zuercher left a comment

zuercher commented Aug 27, 2020

Pchelolo commented Aug 27, 2020

zuercher left a comment

Pchelolo commented Aug 28, 2020

htuch commented Aug 31, 2020

zuercher left a comment

zuercher Sep 1, 2020

Pchelolo Sep 1, 2020

zuercher Sep 1, 2020

Pchelolo Sep 1, 2020

zuercher left a comment

Implement omit_empty_values for access log formatters #12801

Implement omit_empty_values for access log formatters #12801

Conversation

Pchelolo commented Aug 24, 2020

repokitteh-read-only bot commented Aug 24, 2020

zuercher left a comment

Choose a reason for hiding this comment

Pchelolo commented Aug 25, 2020

Pchelolo commented Aug 26, 2020

zuercher left a comment

Choose a reason for hiding this comment

Pchelolo commented Aug 26, 2020

zuercher left a comment

Choose a reason for hiding this comment

zuercher commented Aug 27, 2020

Pchelolo commented Aug 27, 2020

zuercher left a comment

Choose a reason for hiding this comment

Pchelolo commented Aug 28, 2020

htuch commented Aug 31, 2020

zuercher left a comment

Choose a reason for hiding this comment

zuercher Sep 1, 2020

Choose a reason for hiding this comment

Pchelolo Sep 1, 2020

Choose a reason for hiding this comment

zuercher Sep 1, 2020

Choose a reason for hiding this comment

Pchelolo Sep 1, 2020

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment