Skip to content

access log: support formatter extensions from json_format#15646

Merged
snowp merged 17 commits intoenvoyproxy:mainfrom
rgs1:access-log-formatters-support-json-format
Mar 30, 2021
Merged

access log: support formatter extensions from json_format#15646
snowp merged 17 commits intoenvoyproxy:mainfrom
rgs1:access-log-formatters-support-json-format

Conversation

@rgs1
Copy link
Member

@rgs1 rgs1 commented Mar 24, 2021

This was missed in #14512.

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

This was missed in envoyproxy#14512.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@lizan
Copy link
Member

lizan commented Mar 24, 2021

@envoyproxy/first-pass-reviewers to take the first pass.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Raul Gutierrez Segales added 4 commits March 24, 2021 14:19
Convenient for other users of the class.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
* extend the existing test to use the extension in multiple parts
  of the JSON doc
* add additional test for multiple extensions

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@dmitri-d
Copy link
Contributor

lgtm, @rgs1, provided CI is happy.

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few comments

Raul Gutierrez Segales added 6 commits March 25, 2021 15:38
TODO(rgs1): please add a clang-tidy check that flags these

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1
Copy link
Member Author

rgs1 commented Mar 25, 2021

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15646 (comment) was created by @rgs1.

see: more, trace.

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks this looks good, just the one comment about a comment

Raul Gutierrez Segales added 2 commits March 29, 2021 14:55
This avoids the dangers around a dangling reference.

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
@rgs1
Copy link
Member Author

rgs1 commented Mar 30, 2021

@snowp ready for another pass -- PTAL. Thanks!

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM just one nit

Raul Gutierrez Segales added 2 commits March 30, 2021 10:57
Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>
Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks!

@snowp snowp merged commit be51d87 into envoyproxy:main Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants