formatter: add a formatter that returns a google::protobuf::Struct rather than a string #14258

itamarkam · 2020-12-03T16:36:01Z

Commit Message: add a formatter that returns google::protobuf::Struct rather than a string
Additional Description: the logic was already implemented in the JSON (string) formatter, which created a formatted struct and converted it into a string. The JSON formatter was split into the aforementioned Struct formatter, and a wrapper that converts the struct into a string. Note that the new formatter is still not used anywhere (will be in the OTLP logger)
Risk Level: Low (refactoring)
Testing: Unit tests (JSON formatter tests were converted to Struct formatter tests and a simpler test was added to the JSON formatter).
Docs Changes: None
Release Notes: None
Platform Specific Features: None
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

@omriz @yanavlasov @htuch @edrukh

repokitteh-read-only · 2020-12-03T16:36:04Z

Hi @itamarkam, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14258 was opened by itamarkam.

see: more, trace.

itamarkam · 2020-12-03T16:40:23Z

@yanavlasov this is a small refactoring, to be used later by the OT logger - I wanted to get the hang of Envoy repo PRs with something relatively simple. If you prefer that I wait with this change and send it together with the actual logger implementation, I'll do that.

test/common/formatter/substitution_formatter_speed_test.cc

zuercher

Thanks! This is just a quick initial pass.

test/common/formatter/substitution_formatter_speed_test.cc

repokitteh-read-only · 2020-12-09T19:30:10Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #14258 was synchronize by itamarkam.

see: more, trace.

… wrapper that generates a string Signed-off-by: Itamar Kaminski <itamark@google.com>

Signed-off-by: Itamar Kaminski <itamark@google.com> Delete some leftovers Signed-off-by: Itamar Kaminski <itamark@google.com> Delete some leftovers Signed-off-by: Itamar Kaminski <itamark@google.com> clang-format Signed-off-by: Itamar Kaminski <itamark@google.com> Revert "clang-format" This reverts commit 4576bc86fdb8baa2f161bde2dbdd7871169f725f. Signed-off-by: Itamar Kaminski <itamark@google.com> Fix error message formatting test Fix clang-tidy Signed-off-by: Itamar Kaminski <itamark@google.com> Fix clang tidy again Signed-off-by: Itamar Kaminski <itamark@google.com>

zuercher

Thanks! Some small comments, but otherwise looks good.

source/common/formatter/substitution_formatter.cc

zuercher · 2020-12-10T19:47:20Z

source/common/formatter/substitution_formatter.h

+  };
+
+  bool omit_empty_values_;
+  bool preserve_types_;


I think these bools can be const as well.

source/common/formatter/substitution_formatter.h

mattklein123 · 2020-12-10T23:12:48Z

I think this was tagged for API remove accidentally so I'm removing myself.

yanavlasov · 2020-12-11T16:18:31Z

LGTM modulo @zuercher comments.

Signed-off-by: Itamar Kaminski <itamark@google.com>

yanavlasov · 2020-12-14T17:26:18Z

/azp run

azure-pipelines · 2020-12-14T17:26:28Z

Azure Pipelines successfully started running 1 pipeline(s).

* master: (49 commits) sds: allow multiple init managers share sds target (envoyproxy#14357) [http] Remove legacy codecs (envoyproxy#14381) http2: Add integration tests for METADATA and RST_STREAM frame flood mitigation for upstream servers (envoyproxy#14365) test: start dissolving :printers_include rule. (envoyproxy#14429) integration tests: re-enable set_node_on_first_message_only (envoyproxy#14270) formatter: add a formatter that returns a google::protobuf::Struct rather than a string (envoyproxy#14258) ratelimit: support returning custom response bodies for non-OK responses from the external ratelimit service (envoyproxy#14189) deps: update protobuf to 3.14 (envoyproxy#14253) stream_info: add setResponseCode and update local_reply to take a normal StreamInfo (envoyproxy#14402) http: alpn upstream (envoyproxy#13922) Moved starttls integration test to test/extensions/transport_sockets/starttls. (envoyproxy#14425) generic conn pool: directly use thread local cluster (envoyproxy#14423) wasm: add mathetake to CODEOWNERS (envoyproxy#14427) wasm: clear route cache when modifying HTTP request headers. (envoyproxy#14318) tls: disable TLS inspector injection (envoyproxy#14404) aggregate cluster: cleanups (envoyproxy#14411) Mark starttls_integration_test flaky on Windows (envoyproxy#14419) tcp: improved unit testing (envoyproxy#14415) config: making protocol config explicit (envoyproxy#14362) wasm: dead code (envoyproxy#14407) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

itamarkam force-pushed the master branch from 4ef3589 to 1f7a262 Compare December 3, 2020 16:37

jmarantz requested a review from zuercher December 5, 2020 15:49

jmarantz assigned zuercher Dec 5, 2020

itamarkam force-pushed the master branch 2 times, most recently from 5d1bd72 to 94d03ab Compare December 6, 2020 15:28

itamarkam commented Dec 6, 2020

View reviewed changes

test/common/formatter/substitution_formatter_speed_test.cc Outdated Show resolved Hide resolved

yanavlasov self-assigned this Dec 8, 2020

itamarkam force-pushed the master branch 2 times, most recently from 5c04629 to 784528a Compare December 8, 2020 17:38

zuercher reviewed Dec 8, 2020

View reviewed changes

test/common/formatter/substitution_formatter_speed_test.cc Outdated Show resolved Hide resolved

itamarkam force-pushed the master branch 4 times, most recently from accf9cb to 5da219b Compare December 9, 2020 19:14

itamarkam requested review from alyssawilk, asraa, dio, ggreenway, htuch, jmarantz, junr03, lambdai, lizan, mattklein123, PiotrSikora and snowp as code owners December 9, 2020 19:14

repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Dec 9, 2020

repokitteh-read-only bot assigned mattklein123 Dec 9, 2020

itamarkam marked this pull request as draft December 9, 2020 19:34

itamarkam force-pushed the master branch 2 times, most recently from 5da219b to 5d1bd72 Compare December 10, 2020 13:14

Split the JSON (string) formatter to a Struct (proto) formatter and a…

6fe09ac

… wrapper that generates a string Signed-off-by: Itamar Kaminski <itamark@google.com>

itamarkam force-pushed the master branch from 5d1bd72 to 71513d3 Compare December 10, 2020 13:19

itamarkam force-pushed the master branch from 71513d3 to 7055084 Compare December 10, 2020 13:39

itamarkam marked this pull request as ready for review December 10, 2020 13:42

itamarkam requested a review from zuercher December 10, 2020 13:43

zuercher reviewed Dec 10, 2020

View reviewed changes

mattklein123 removed api deps Approval required for changes to Envoy's external dependencies labels Dec 10, 2020

mattklein123 removed their assignment Dec 10, 2020

Review fixes

e2675c3

Signed-off-by: Itamar Kaminski <itamark@google.com>

itamarkam requested a review from zuercher December 11, 2020 19:01

Fix error message typo

67bc523

Signed-off-by: Itamar Kaminski <itamark@google.com>

yanavlasov previously approved these changes Dec 14, 2020

View reviewed changes

Merge branch 'master' into master

ca6345c

itamarkam dismissed yanavlasov’s stale review via ca6345c December 15, 2020 11:12

yanavlasov approved these changes Dec 16, 2020

View reviewed changes

yanavlasov merged commit ba656eb into envoyproxy:master Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

formatter: add a formatter that returns a google::protobuf::Struct rather than a string #14258

formatter: add a formatter that returns a google::protobuf::Struct rather than a string #14258

itamarkam commented Dec 3, 2020 •

edited

repokitteh-read-only bot commented Dec 3, 2020

itamarkam commented Dec 3, 2020

zuercher left a comment

repokitteh-read-only bot commented Dec 9, 2020

zuercher left a comment

zuercher Dec 10, 2020 •

edited

mattklein123 commented Dec 10, 2020

yanavlasov commented Dec 11, 2020 •

edited

yanavlasov commented Dec 14, 2020

azure-pipelines bot commented Dec 14, 2020

formatter: add a formatter that returns a google::protobuf::Struct rather than a string #14258

formatter: add a formatter that returns a google::protobuf::Struct rather than a string #14258

Conversation

itamarkam commented Dec 3, 2020 • edited

repokitteh-read-only bot commented Dec 3, 2020

itamarkam commented Dec 3, 2020

zuercher left a comment

Choose a reason for hiding this comment

repokitteh-read-only bot commented Dec 9, 2020

zuercher left a comment

Choose a reason for hiding this comment

zuercher Dec 10, 2020 • edited

Choose a reason for hiding this comment

mattklein123 commented Dec 10, 2020

yanavlasov commented Dec 11, 2020 • edited

yanavlasov commented Dec 14, 2020

azure-pipelines bot commented Dec 14, 2020

itamarkam commented Dec 3, 2020 •

edited

zuercher Dec 10, 2020 •

edited

yanavlasov commented Dec 11, 2020 •

edited