Add support for nested JSON format in json logging mode by Pchelolo · Pull Request #12602 · envoyproxy/envoy

Pchelolo · 2020-08-12T01:05:23Z

This adds support for nesting in JSON logging mode. There is another opportunity for improvement - support static non-string values, but I guess we can do this separately.

Commit Message: Add support for nested JSON format in JSON logging mode.
Risk Level: Low, feature only used if nested objects are configured, thus opt-in.
Testing: unit tests
Docs Changes: yes
Release Notes: added to current.rst
Fixes #12582

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

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

zuercher

Thanks!

Have you run the substitution_formatter_speed_test before/after this change? It doesn't seem like it should change the performance for flat json formats, but I'd like to confirm.

source/common/formatter/substitution_formatter.cc

source/common/formatter/substitution_formatter.h

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

Pchelolo · 2020-08-13T22:23:22Z

@zuercher thank you for the review, updated according to your comments.

aa-stripe · 2020-08-13T22:46:02Z

🎉

happy to see this going through so quickly!

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

zuercher · 2020-08-14T20:18:41Z

Can you run the speed test noted above and confirm there’s no significant degradation? Thanks.

Pchelolo · 2020-08-14T23:02:29Z

Oh, sorry @zuercher, I've totally missed your comment about the speed test!

master

-------------------------------------------------------------------------
Benchmark                               Time             CPU   Iterations
-------------------------------------------------------------------------
BM_AccessLogFormatter               12882 ns        12795 ns        55076
BM_JsonAccessLogFormatter          280000 ns       278318 ns         2459
BM_TypedJsonAccessLogFormatter     260001 ns       258375 ns         2635

feature branch

-------------------------------------------------------------------------
Benchmark                               Time             CPU   Iterations
-------------------------------------------------------------------------
BM_AccessLogFormatter               10059 ns        10038 ns        71441
BM_JsonAccessLogFormatter          284222 ns       283625 ns         2468
BM_TypedJsonAccessLogFormatter     277584 ns       277049 ns         2535

The numbers seem comparable.

zuercher

Thanks!

cc @dio for a second pass

dio

Thanks, very well done! Few comments:

dio · 2020-08-20T01:28:10Z

source/common/formatter/substitution_formatter.cc


+JsonFormatterImpl::JsonFormatMap
+JsonFormatterImpl::toFormatMap(const ProtobufWkt::Struct& json_format) const {
+  auto output = std::make_unique<std::map<std::string, JsonFormatterImpl::JsonFormatMapValue>>();


I assume we want to preserve order here? Do you mind adding comments here so we understand why we choose std::map?

Done. Given that I've made aliases per the other comment, the explanation comment is now in the header file where the type's defined.

dio · 2020-08-20T01:30:37Z

source/common/formatter/substitution_formatter.h

+  using JsonFormatMapValue =
+      absl::variant<const std::vector<FormatterProviderPtr>, const JsonFormatMap>;
+  struct JsonFormatMap {
+    std::unique_ptr<std::map<std::string, JsonFormatMapValue>> value_;


We usually have a typedef for this. But I don't feel strongly about it.

Added some aliases. Looks more readable indeed now.

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

dio · 2020-08-20T06:14:41Z

source/common/formatter/substitution_formatter.h

+  using JsonFormatMapValue =
+      absl::variant<const std::vector<FormatterProviderPtr>, const JsonFormatMapWrapper>;
+  // Although not required for JSON, it is nice to have the order of properties
+  // preserved between the format and the log entry, thus std::map


Since you need to fix format anyway, please end this with full-stop. Thanks.

dio · 2020-08-20T06:17:40Z

source/common/formatter/substitution_formatter.h

+  // preserved between the format and the log entry, thus std::map
+  using JsonFormatMap = std::map<std::string, JsonFormatMapValue>;
+  using JsonFormatMapPtr = std::unique_ptr<JsonFormatMap>;
+  struct JsonFormatMapWrapper{


Here. Missing space between JsonFormatMapWrapper and {.

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

dio · 2020-08-20T16:32:30Z

/retest

repokitteh-read-only · 2020-08-20T16:32:35Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #12602 (comment) was created by @dio.

see: more, trace.

dio

Thank you!

* envoy/master: (90 commits) cleanup: use structured binding (envoyproxy#12791) docs: fix header name for retries in gRPC services (envoyproxy#12790) docs: clarify meaning of HeaderValueOption.append (envoyproxy#12792) doc: clarify handling of duplicate xDS resource names (envoyproxy#12756) Dependencies: build updates. (envoyproxy#12786) Ratelimit: Add optional descriptor key to generic_key action (envoyproxy#12734) test: refactor header inclusion to speed up building (for test/mocks/upstream:upstream_mocks) (envoyproxy#12407) docs: Fix omitted word (envoyproxy#12782) ci: avoid uploading dwp as separate artifact (envoyproxy#12777) doc: Fix small typos (envoyproxy#12769) fix cache factory category (envoyproxy#12765) docs: fix typo v1.15.0.rst (envoyproxy#12680) Add clang-cl RBE toolchain for Windows (envoyproxy#12776) fuzz: add router fuzz proto (envoyproxy#12727) header: New HeaderMatcher and StringMatcher type - Contains (envoyproxy#12623) tcp_proxy: use dynamicMetadata() from StreamInfo for load balancing (envoyproxy#12595) network: add io handle recv function for http inspector (envoyproxy#12736) jwt_authn: supports jwt payload without "iss" field (envoyproxy#12744) Add support for nested JSON format in json logging mode (envoyproxy#12602) http: fixing a fuzz flake by setting details on connection teardown (envoyproxy#12737) ... Signed-off-by: Scott LaVigne <lavignes@amazon.com>

Petr Pchelko added 3 commits August 10, 2020 21:15

TEMP

2586857

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

Support nested objects in access logging json format

b15125c

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

Revert removal of createJsonFormatter

f36fe96

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

jmarantz assigned zuercher Aug 12, 2020

zuercher reviewed Aug 13, 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/formatter/substitution_formatter.h Outdated Show resolved Hide resolved

Improve exception message, make JsonFormatMap private

9b0df3f

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

Fix format

aed8788

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

zuercher previously approved these changes Aug 17, 2020

View reviewed changes

zuercher assigned dio Aug 17, 2020

Pchelolo mentioned this pull request Aug 20, 2020

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

Closed

dio reviewed Aug 20, 2020

View reviewed changes

Add more comments and typedefs

8e3ad25

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

Pchelolo dismissed zuercher’s stale review via 8e3ad25 August 20, 2020 04:49

dio reviewed Aug 20, 2020

View reviewed changes

Fix format

cabef01

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

dio approved these changes Aug 20, 2020

View reviewed changes

zuercher approved these changes Aug 21, 2020

View reviewed changes

zuercher merged commit 04ce876 into envoyproxy:master Aug 21, 2020

Conversation

Pchelolo commented Aug 12, 2020 • edited by dio Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Pchelolo commented Aug 13, 2020

Uh oh!

aa-stripe commented Aug 13, 2020

Uh oh!

zuercher commented Aug 14, 2020

Uh oh!

Pchelolo commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

dio Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pchelolo Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

dio Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

Pchelolo Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

dio Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dio Aug 20, 2020

Choose a reason for hiding this comment

Uh oh!

dio commented Aug 20, 2020

Uh oh!

repokitteh-read-only bot commented Aug 20, 2020

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Pchelolo commented Aug 12, 2020 •

edited by dio

Loading

Pchelolo commented Aug 14, 2020 •

edited

Loading

dio Aug 20, 2020 •

edited

Loading

dio Aug 20, 2020 •

edited

Loading