access log: add response flag filter by junr03 · Pull Request #3536 · envoyproxy/envoy

junr03 · 2018-06-04T18:55:11Z

access log: add response flag filter

After #3299 merged log lines for gRPC requests that would have resulted in non-200 HTTP status codes in the past (for instance an unroutable request 404 and NR response flag), now result in a 200 HTTP status code. This can potentially filter out log lines that were previously being logged.

This PR adds an access log filter, that prints log lines if an Envoy response flag is set.

Testing: unit tests for the access log filter, and a new API call for the request info class.

Docs Changes:
Added documentation to the filter. And release notes.

Release Notes:
Added release notes.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2018-06-04T18:56:13Z

api/envoy/config/filter/accesslog/v2/accesslog.proto

+// A list of the response flags can be found
+// in the access log formatter :ref:`documentation<config_access_log_format_response_flags>`.
+message ResponseFlagFilter {
+}


alternatively I thought about adding an option to specify the flags you care about. But I think that response flags are important, and infrequent enough to have the filter filter on all of them. Thoughts?

One way to achieve both settings is have an array field that will specify which response flags to log on and if that field is empty default to all response flags? (or have an enum that says all)

Yeah +1 to what @ccaraman said. I would do that.

Yep, that is what I thought, will add.

junr03 · 2018-06-04T18:56:36Z

@zuercher can you do a first pass?

zuercher · 2018-06-05T20:10:15Z

Nothing jumps out -- I'll have another look when you update with the requested change.

junr03 · 2018-06-08T00:49:51Z

quick update here: taking a bit longer because I needed to take a wide detour to protoc-gen-validate. I want to add a rule to the flags field like:
repeated string flags = 1 [(validate.rules).repeated.items.string = {in: ["foo", "bar"]}];
but there is a bug in protoc-gen-validate that is generating incorrect code. Will update here once I fix there.

PR in pgv fixing https://github.com/lyft/protoc-gen-validate/pull/78

Signed-off-by: Jose Nino <jnino@lyft.com>

stale · 2018-06-18T20:39:28Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2018-06-21T02:38:53Z

@zuercher sorry for the delay here. PR is now up to date.

zuercher

I think it needs some extra testing but otherwise looks good.

zuercher · 2018-06-21T15:38:38Z

include/envoy/request_info/request_info.h

+  /**
+   * @return whether any response flag is set or not.
+   */
+  virtual bool getResponseFlag() const PURE;


nit: should this be hasResponseFlag()?

yeah I think that is a better name, and also a better name for the other function getResponseFlag(ResponseFlag response_flag). I wanted to preserve naming. But now that you brought it up again I think it is probably better for clarity to change both names.

zuercher · 2018-06-21T15:56:14Z

source/common/access_log/access_log_impl.cc

+    absl::optional<RequestInfo::ResponseFlag> response_flag =
+        RequestInfo::ResponseFlagUtils::toResponseFlag(config.flags(i));
+    // The config has been validated. Therefore, every flag in the config will have a mapping.
+    RELEASE_ASSERT(response_flag.has_value());


At this point the set of flags is reproduced in a few places:

RequestInfo::ResponseFlag

ResponseFlagUtils

the ResponseFlagFilter proto message's validation

I think it would be good to have a test that validates each response flag to prove that the list in the validation is correct, and a similar test for toResponseFlag to prove that each flag is in the static map there.

👌 will add

nit, would probably switch this to normal ASSERT

Signed-off-by: Jose Nino <jnino@lyft.com>

zuercher · 2018-06-25T16:43:30Z

Looks good. I'll approve once the merge conflict is fixed.

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2018-06-25T19:38:07Z

@zuercher thanks. The conflict has been resolved but the build is going to fail because I moved the PGV sha, and we haven't merged the necessary fix yet (https://github.com/lyft/protoc-gen-validate/pull/78). I'll ping you when that gets merged.

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123

LGTM, just a few small comments. Nice!

mattklein123 · 2018-06-27T19:53:29Z

api/envoy/config/filter/accesslog/v2/accesslog.proto

+// in the access log formatter :ref:`documentation<config_access_log_format_response_flags>`.
+message ResponseFlagFilter {
+  // Only responses with the any of the flags listed in this field will be logged.
+  // This field is optional if it is not specified, then any response flag will pass


nit: "This field is optional. If it ..."

mattklein123 · 2018-06-27T19:53:43Z

docs/root/intro/version_history.rst

 * access log: added ability to format START_TIME.
 * access log: added DYNAMIC_METADATA :ref:`access log formatter <config_access_log_format>`.
 * access log: added :ref:`HeaderFilter <envoy_api_msg_config.filter.accesslog.v2.HeaderFilter>`
+  to filter logs based on request headers


Move to 1.8.0 section.

mattklein123 · 2018-06-27T19:54:32Z

source/common/access_log/access_log_impl.cc

+    absl::optional<RequestInfo::ResponseFlag> response_flag =
+        RequestInfo::ResponseFlagUtils::toResponseFlag(config.flags(i));
+    // The config has been validated. Therefore, every flag in the config will have a mapping.
+    RELEASE_ASSERT(response_flag.has_value());


nit, would probably switch this to normal ASSERT

mattklein123 · 2018-06-27T19:55:37Z

source/common/access_log/access_log_impl.h

+                const Http::HeaderMap& request_headers) override;
+
+private:
+  RequestInfo::RequestInfoImpl info_;


nit: It seems a little odd to me to have a full request info here, but it's probably fine. Just throwing that out there.

Yeah, it is a little clunky. I guess I could just maintain the uint64_t for the flags and instead of using the setResponseFlag abstraction do the |= myself.

That way I also can get rid of the currentResponseFlags() method and add less stuff to that api.

wdyt?

yeah I would probably do that...

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2018-06-28T20:48:35Z

@mattklein123 comments are addressed.

mattklein123

LGTM, small comments.

mattklein123 · 2018-06-28T21:28:11Z

docs/root/intro/version_history.rst

 1.8.0 (Pending)
 ===============
+* access log: added :ref:`response flag filter <envoy_api_msg_config.filter.accesslog.v2.ResponseFlagFilter>` to filter based on the presence of Envoy response flags. 
+  to filter logs based on request headers.


duplicated text / please break on 100 chars

mattklein123 · 2018-06-28T21:33:16Z

source/common/access_log/access_log_impl.h


 #include "common/http/header_utility.h"
 #include "common/protobuf/protobuf.h"
+#include "common/request_info/request_info_impl.h"


This header change can be reverted I believe here and above.

mattklein123 · 2018-06-28T21:35:42Z

include/envoy/request_info/request_info.h

+  /**
+   * @return whether any response flag is set or not.
+   */
+  virtual bool hasResponseFlag() const PURE;


IMO this function name is a little strange given the other overload of same name Can we call it hasAnyResponseFlag() or something similar?

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 · 2018-06-29T00:09:04Z

@mattklein123 updated

mattklein123

nice

Jose Nino added 6 commits June 1, 2018 16:42

access log: add response flag filter

1176b61

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

d379e04

Signed-off-by: Jose Nino <jnino@lyft.com>

docs

05eb710

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'master' into response-flag-log-filter

4cb95e0

Signed-off-by: Jose Nino <jnino@lyft.com>

fmt

0b4b631

Signed-off-by: Jose Nino <jnino@lyft.com>

merge conflict

d02b862

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 commented Jun 4, 2018

View reviewed changes

junr03 requested a review from zuercher June 4, 2018 18:56

ccaraman assigned zuercher Jun 5, 2018

lizan mentioned this pull request Jun 7, 2018

Mixer proxy access log should indicate errors istio/istio#5429

Closed

wip

53697d5

Signed-off-by: Jose Nino <jnino@lyft.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 18, 2018

Jose Nino added 3 commits June 20, 2018 14:34

wip

e5ad2c8

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'master' into response-flag-log-filter

4bb4afb

Signed-off-by: Jose Nino <jnino@lyft.com>

finish up

c421d95

Signed-off-by: Jose Nino <jnino@lyft.com>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 21, 2018

docs

aa5b20d

Signed-off-by: Jose Nino <jnino@lyft.com>

zuercher requested changes Jun 21, 2018

View reviewed changes

wip test

1c99b8a

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'master' into response-flag-log-filter

bc18e7b

Signed-off-by: Jose Nino <jnino@lyft.com>

update pgv sha

53662e8

Signed-off-by: Jose Nino <jnino@lyft.com>

zuercher previously approved these changes Jun 26, 2018

View reviewed changes

mattklein123 reviewed Jun 27, 2018

View reviewed changes

Jose Nino added 2 commits June 28, 2018 11:00

comments

3421855

Signed-off-by: Jose Nino <jnino@lyft.com>

Merge branch 'master' into response-flag-log-filter

64803c0

Signed-off-by: Jose Nino <jnino@lyft.com>

junr03 dismissed zuercher’s stale review via 64803c0 June 28, 2018 19:33

Jose Nino added 2 commits June 28, 2018 12:37

docs

9ebe110

Signed-off-by: Jose Nino <jnino@lyft.com>

period

08bb66e

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 reviewed Jun 28, 2018

View reviewed changes

comments

779af08

Signed-off-by: Jose Nino <jnino@lyft.com>

mattklein123 approved these changes Jun 29, 2018

View reviewed changes

mattklein123 merged commit 9365720 into master Jul 2, 2018

mattklein123 deleted the response-flag-log-filter branch July 2, 2018 21:59

Conversation

junr03 commented Jun 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jun 4, 2018

Uh oh!

zuercher commented Jun 5, 2018

Uh oh!

junr03 commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stale bot commented Jun 18, 2018

Uh oh!

junr03 commented Jun 21, 2018

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuercher commented Jun 25, 2018

Uh oh!

junr03 commented Jun 25, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jun 28, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junr03 commented Jun 29, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

junr03 commented Jun 8, 2018 •

edited

Loading