Cross platform redirect the access_log output to (stderr/stdout) by davinci26 · Pull Request #15202 · envoyproxy/envoy

davinci26 · 2021-02-26T00:56:59Z

Commit Message:
Fixes: #13964

Additional Description:

Risk Level: Medium
Testing: Unit/Integration and Manual validation
Docs Changes: Completed
Release Notes: add access_log_destination property to FileAccessLog to write to standard streams in a cross platform way.
Platform Specific Features: N/A

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

repokitteh-read-only · 2021-02-26T00:57:06Z

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/.

🐱

Caused by: #15202 was opened by davinci26.

see: more, trace.

davinci26 · 2021-02-26T00:57:31Z

Mostly a WiP, my VM can't compile and run all the tests so I have to iterate over the CI a bit. Will update when its ready for a review

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2021-03-02T00:51:58Z

@antoniovicente @jmarantz since both of you were involved in the discussions, do you want to do the code review?

source/common/access_log/BUILD

davinci26 · 2021-03-02T05:14:53Z

api/envoy/extensions/access_loggers/file/v3/file.proto

      "envoy.config.accesslog.v2.FileAccessLog";

-  // A path to a local file to which to write the access log entries.
-  string path = 1 [(validate.rules).string = {min_len: 1}];


The reason we don't wrap these two properties into a struct and make them oneof is to maintain backwards compatibility

Stepping back for a sec, do you think it makes sense for a "file access log" to output to console, stderr, stdout, etc.? Or should we have ConsoleAccessLog, StderrAccessLog, StdoutAccessLog?

If we think putting this functionality in FileAccessLog makes sense, here is what I would suggest:

Deprecate path

Add a new oneof "output_type" or similar

Define a new message for each option: Path, Console, Stderr, Stdout (this allows for future extensibility)

I would then use ^ in the admin proto (and deprecate path their as well and/or potentially even allow loading full access log extensions so we can instantiate grpc access log, file access log, etc.). It's unfortunate that admin is not a full listener but that is a well known issue and I'm not suggesting we fix that here.

I like the idea of creating ConsoleAccessLog, StderrAccessLog, StdoutAccessLog and using it everywhere.

Can I deprecate the path in admin and replace it with a config_type that accepts on of the configs above? Or I can just add the config_type property in admin.

What I would do is deprecate path in admin and actually make it a repeated list of access log extensions the same as we use elsewhere. You will see it will be trivial to wire up in the fake admin listener.

test/common/filesystem/filesystem_impl_test.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2021-03-08T19:40:11Z

@antoniovicente I did some updates, added docs and expect the CI to go green. Feel free to take another look when you get some time.

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente

Looks great, just a few nits and some small requests for additional tests.

source/common/config/utility.cc

test/common/access_log/access_log_impl_test.cc

test/common/filesystem/filesystem_impl_test.cc

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente

Looks great.

@mattklein123 off to you for API review.

davinci26 · 2021-03-10T01:20:20Z

thanks for the thoughtful review @antoniovicente

mattklein123

Very cool. I'm sorry to have not looked at this sooner, but some high level API comments to discuss. Thank you.

/wait

mattklein123 · 2021-03-10T18:55:50Z

api/envoy/extensions/access_loggers/file/v3/file.proto

      "envoy.config.accesslog.v2.FileAccessLog";

-  // A path to a local file to which to write the access log entries.
-  string path = 1 [(validate.rules).string = {min_len: 1}];


Stepping back for a sec, do you think it makes sense for a "file access log" to output to console, stderr, stdout, etc.? Or should we have ConsoleAccessLog, StderrAccessLog, StdoutAccessLog?

If we think putting this functionality in FileAccessLog makes sense, here is what I would suggest:

Deprecate path

Add a new oneof "output_type" or similar

Define a new message for each option: Path, Console, Stderr, Stdout (this allows for future extensibility)

I would then use ^ in the admin proto (and deprecate path their as well and/or potentially even allow loading full access log extensions so we can instantiate grpc access log, file access log, etc.). It's unfortunate that admin is not a full listener but that is a well known issue and I'm not suggesting we fix that here.

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 · 2021-03-11T18:01:26Z

Closing to break it down into two PRs

Sotiris Nanopoulos added 2 commits February 25, 2021 15:58

unix attempt 1

316a8b8

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

format changes

92bc8a3

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

repokitteh-read-only bot added the api label Feb 26, 2021

repokitteh-read-only bot assigned mattklein123 Feb 26, 2021

Sotiris Nanopoulos added 11 commits February 25, 2021 18:04

fix admin_test

ae1dc84

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix conn_manager_impl_test_base

69da6e7

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix format

e1dffcf

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix filesystem mock

be36399

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix config_test

833a8c2

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Adds a Windows implementation

b15411f

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix build break

df791b2

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix build break

021d444

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Merge remote-tracking branch 'upstream/main' into accesslogtype

9e07c2b

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

remove comments

b79082f

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Merge remote-tracking branch 'upstream/main' into accesslogtype

fb7fa6e

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

mattklein123 added the waiting label Mar 1, 2021

Sotiris Nanopoulos added 3 commits March 1, 2021 11:01

Add tests for bootstrap

c535e55

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

adds integration tests

e5590e3

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

format

6f7e05c

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

repokitteh-read-only bot removed the waiting label Mar 1, 2021

Sotiris Nanopoulos added 2 commits March 1, 2021 15:35

Windows implementation

502a10c

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

remove commented out code

0b5b2c2

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 commented Mar 2, 2021

View reviewed changes

source/common/access_log/BUILD Outdated Show resolved Hide resolved

davinci26 commented Mar 2, 2021

View reviewed changes

test/common/filesystem/filesystem_impl_test.cc Outdated Show resolved Hide resolved

fix gcc issues

8ef5bc6

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente self-assigned this Mar 2, 2021

Sotiris Nanopoulos added 12 commits March 5, 2021 08:56

merge

3d886c7

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

add empty line

48091eb

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

change from std::string_view to absl::string_view

510f925

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

adds default constructor

80e07d1

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix typo

b1bc42b

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

remove unused this parameter

9153bc4

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

trigger ci

132363c

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

clang-tidy, coverage and docs

fe277f9

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

docs format

93b5eec

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Merge remote-tracking branch 'upstream/main' into accesslogtype

846c799

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix docs

4e0ba71

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

docs order

2103379

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix server config bug

1bf0a75

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente reviewed Mar 9, 2021

View reviewed changes

Sotiris Nanopoulos added 2 commits March 8, 2021 17:00

address pr comments

4b20da9

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

revert change in switch

0358997

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

antoniovicente previously approved these changes Mar 10, 2021

View reviewed changes

mattklein123 requested changes Mar 10, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 10, 2021

PR comments cascading

eaab59a

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

davinci26 dismissed antoniovicente’s stale review via eaab59a March 11, 2021 18:01

davinci26 requested a review from zuercher as a code owner March 11, 2021 18:01

repokitteh-read-only bot removed the waiting label Mar 11, 2021

davinci26 closed this Mar 11, 2021

This was referenced Mar 12, 2021

Adds Access loggers to admin interface #15461

Merged

Stream access logs #15494

Merged

wrowe mentioned this pull request Mar 19, 2021

(Windows) Cannot Redirect the access_log output to (stderr/stdout/null) #13964

Closed

Conversation

davinci26 commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 26, 2021

Uh oh!

davinci26 commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davinci26 commented Mar 2, 2021

Uh oh!

Uh oh!

davinci26 Mar 2, 2021

Choose a reason for hiding this comment

Uh oh!

mattklein123 Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

davinci26 Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

mattklein123 Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davinci26 commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

davinci26 commented Mar 10, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

davinci26 commented Mar 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

davinci26 commented Feb 26, 2021 •

edited

Loading

davinci26 commented Feb 26, 2021 •

edited

Loading

davinci26 commented Mar 8, 2021 •

edited

Loading