access log: Add AccessLoggers::Common::ImplBase base class to handle AccessLog::Filter evaluation by aunu53 · Pull Request #7359 · envoyproxy/envoy

aunu53 · 2019-06-21T20:43:06Z

Introduce AccessLoggers::Common::ImplBase in the access_loggers extension to handle converting request/response/trailer HeaderMap pointer-to-reference conversion and evaluation of access log filters. Refactors existing loggers to inherit from the common base class.

Risk Level: Low. No behavior change intended.
Testing: Added unit tests for base class, existing tests suffice for other loggers.
Docs Changes: n/a
Release Notes: n/a
Fixes: #7325

Signed-off-by: Auni Ahsan auni@google.com

Signed-off-by: Auni Ahsan <auni@google.com>

include/envoy/access_log/access_log.h

…aderMap& Signed-off-by: Auni Ahsan <auni@google.com>

Signed-off-by: Auni Ahsan <auni@google.com>

zuercher

I think this is getting pretty close.

include/envoy/access_log/access_log.h

source/common/access_log/access_log_impl.h

aunu53 · 2019-06-28T18:42:24Z

Almost ready to put up the Base class, I'm just getting stuck on this stupid CODEOWNERS error when I try to lint myself.

> (cd ~/envoy && ./ci/run_envoy_docker.sh './ci/do_ci.sh fix_format')
No remote cache bucket is set, skipping setup remote cache.
ENVOY_SRCDIR=/source
find: File system loop detected; '/source/.cache/bazel/_bazel_envoybuild/b570b5ccd0454dc9af9f65ab1833764d/external/envoy' is part of the same file system loop as '/source'.
building using 12 CPUs
fix_format...
ERROR: New directory extensions/access_loggers/common appears to not have owners in CODEOWNERS
ERROR: check format failed. run 'tools/check_format.py fix'

I tried adding /*/extensions/access_loggers/common @auni, but no dice.

zuercher · 2019-06-29T03:14:51Z

Was looking at this earlier but got sidetracked. I think it’s a bug in the script. If you want to take a look at fixing it, it’d be much appreciated. Otherwise I’ll try to fix it up on Monday.

zuercher · 2019-07-01T20:26:06Z

@auni53 I see the problem. It's not really a bug so much as failure to emit the correct error. Each directory requires two owners. So go ahead and add me as the second one, I guess.

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-07-02T13:09:44Z

Sorry I was having computer problems, just running the test suite and I'll have an update on this soon.

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-07-03T20:04:58Z

Ready for more review! I left the log() interface alone but changed emitLog to references. Think I should change the log() function too? (I'd probably do that in a separate PR.) If not, let me know if I should add a .cc as well.

Since I'm not currently changing any base interfaces, do I need release notes?

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-07-08T23:36:21Z

Sorry for the churn, was trying and failing to clean up the dependency tree. (Envoy unfortunately does not enforce "include what you use".) This is ready for another review round now!

zuercher

No worries. Thanks for cleaning up the dependencies.

include/envoy/access_log/access_log.h

source/extensions/filters/http/tap/tap_filter.h

source/extensions/access_loggers/common/access_log_base.h

Signed-off-by: Auni Ahsan <auni@google.com>

zuercher

Looks good! Thanks for sticking with this.

I went ahead and updated the PR description (which we'll use as the commit message on merge) to match the PR in this state. Feel free to make edits.

jmarantz

It seems like this PR is intended to enable sharing of some meaty code that's not shared now. What code is that? Maybe just add that to the PR description?

jmarantz · 2019-07-10T20:21:42Z

include/envoy/server/access_log_config.h

-   * @param config the custom configuration for this access log type.
-   * @param filter filter to determine whether a particular request should be logged. If no filter
+   * @param config is the custom configuration for this access log type.
+   * @param filter can determine whether a particular request should be logged. If no filter


I think the previous state is more doxygen style at least as used in Envoy; it's not a sentence; it's the arg name followed by a description of the arg. For me, you can resolve this by simply reverting this file.

Ah, I was trying to be consistent with the rest of the codebase. I had just done a search for @param and found, e.g. in header_map.h, @param key specifies the name of the header to set; it WILL NOT be copied. and @param value specifies the value of the header to set; it WILL be copied.

Not strongly opinionated on this and happy to revert, but the inconsistency is not good.

jmarantz · 2019-07-10T20:23:44Z

source/extensions/access_loggers/common/access_log_base.h

+  virtual void log(const Http::HeaderMap* request_headers, const Http::HeaderMap* response_headers,
+                   const Http::HeaderMap* response_trailers,
+                   const StreamInfo::StreamInfo& stream_info) override {
+    static Http::HeaderMapImpl empty_headers;


static non-pod. Can you use a lazy-init?

https://isocpp.org/wiki/faq/ctors#static-init-order

also make it const, which I think will work with evaluate's signature. But I wouldn't want to pass it to something that might modify it if it gets changed in the future.

Hah, glad I'm grouping this code in one place. This "static Http::HeaderMapImpl" was in a couple places. Done.

jmarantz · 2019-07-10T20:38:16Z

source/extensions/access_loggers/common/access_log_base.h

+      return;
+    }
+    return emitLog(*request_headers, *response_headers, *response_trailers, stream_info);
+  }


this seems like too large a function to inline into a .h as a virtual function. Put it into a .cc?

jmarantz · 2019-07-10T20:38:35Z

source/extensions/access_loggers/common/access_log_base.h

+  /**
+   * Log a completed request if the underlying AccessLog `filter_` allows it.
+   */
+  virtual void log(const Http::HeaderMap* request_headers, const Http::HeaderMap* response_headers,


s/virtual//

No need to specify 'virtual' if we have override.

Ah I'd wanted to give overriders the option to override the log() functionality, but I guess that defeats the point of the whole base class. Done.

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 · 2019-07-11T19:25:20Z

Review comments addressed. The shared code isn't meaty but rather sensitive, the "Accesslog::Filter evaluation" mentioned in the PR.

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz · 2019-07-11T20:30:06Z

source/common/access_log/BUILD

    ],
 )
+
+envoy_cc_library(


Looks like these 2 stanzas were moved for no reason. Revert? Or at least move them back above access_log_lib so the diffs are easier to see.

I moved intentionally; access_log_lib is the main dependency people will be consuming so it makes more sense for it to be at the top IMO.

jmarantz · 2019-07-11T20:32:43Z

source/extensions/access_loggers/common/access_log_base.cc

+void ImplBase::log(const Http::HeaderMap* request_headers, const Http::HeaderMap* response_headers,
+                   const Http::HeaderMap* response_trailers,
+                   const StreamInfo::StreamInfo& stream_info) {
+  const Http::HeaderMapImpl empty_headers;


I'm OK with this but it might be worth it to use a lazy singleton; you just need it to be a const pointer, rather than a non-const value. The reason is that (I believe) HeaderMapImpl is not a trivial ctor and it is better not to have to construct it on every log request.

Wrapped this in a ConstSingleton instead.

jmarantz · 2019-07-11T20:39:44Z

source/extensions/access_loggers/http_grpc/grpc_access_log_impl.h

+  // Common::ImplBase
+  void emitLog(const Http::HeaderMap& request_headers, const Http::HeaderMap& response_headers,
+               const Http::HeaderMap& response_trailers,
+               const StreamInfo::StreamInfo& stream_info) override;


why private? usually override methods should be public or protected.

The public interface is in AccessLog::Instance::log(), which calls this overridden emitLog() function. This way logs won't be emitted without a filter evaluation.

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz · 2019-07-16T12:47:48Z

source/extensions/access_loggers/common/access_log_base.cc

                   const Http::HeaderMap* response_trailers,
                   const StreamInfo::StreamInfo& stream_info) {
-  const Http::HeaderMapImpl empty_headers;
+  ConstSingleton<Http::HeaderMapImpl> empty_headers;


I think it's potentially slightly slower to write this using the ConstSingleton abstraction rather than writing it directly because you are potentially calling get() multiple times in the function body., each of which has to do some kind atomic lookup. If you just write the code directly it will only do the atomic update once. But I'm OK with this.

And I guess this method is faster if all three forms of headers is specified, since then the atomic update will not happen at all :)

But my guess is that in most cases there will be response and request headers but no trailers so ithe lookup will happen once, and it'll be a wash :)

mattklein123 assigned zuercher Jun 22, 2019

Auni Ahsan added 2 commits June 22, 2019 14:39

Add AccessLog::maybeLog

596e20f

Signed-off-by: Auni Ahsan <auni@google.com>

add braces

dd5bb0d

Signed-off-by: Auni Ahsan <auni@google.com>

zuercher reviewed Jun 24, 2019

View reviewed changes

include/envoy/access_log/access_log.h Outdated Show resolved Hide resolved

Change AccessLog::Filter::evaluate() to take HeaderMap* instead of He…

b24e25b

…aderMap& Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 force-pushed the filter branch from 52f5e61 to b24e25b Compare June 24, 2019 18:36

Auni Ahsan added 7 commits June 25, 2019 09:28

fix clang-tidy

163dfcb

Signed-off-by: Auni Ahsan <auni@google.com>

Merge remote-tracking branch 'envoy/master' into filter

1159b63

Signed-off-by: Auni Ahsan <auni@google.com>

Introduce AccessLog::InstanceImpl

9ced41e

Signed-off-by: Auni Ahsan <auni@google.com>

Fix format

37c3438

Signed-off-by: Auni Ahsan <auni@google.com>

Fix compile error

2580ec2

Signed-off-by: Auni Ahsan <auni@google.com>

Fix tap filter

8035a14

Signed-off-by: Auni Ahsan <auni@google.com>

fix mock references

d6b107a

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 changed the title ~~(access log) Add AccessLog::maybeLog to handle AccessLog::Filter evaluation~~ (access log) Add AccessLog::InstanceImpl base class to handle AccessLog::Filter evaluation Jun 25, 2019

zuercher reviewed Jun 25, 2019

View reviewed changes

Auni Ahsan added 2 commits July 2, 2019 08:50

PR changes

afed2b5

Signed-off-by: Auni Ahsan <auni@google.com>

Merge remote-tracking branch 'envoy/master' into filter

2673d0e

Signed-off-by: Auni Ahsan <auni@google.com>

Auni Ahsan added 5 commits July 3, 2019 19:33

add code owner

00c9e6d

Signed-off-by: Auni Ahsan <auni@google.com>

change some signatures

3eca1cf

Signed-off-by: Auni Ahsan <auni@google.com>

fix more interface

6764d79

Signed-off-by: Auni Ahsan <auni@google.com>

final PR changes

33dc965

Signed-off-by: Auni Ahsan <auni@google.com>

Merge remote-tracking branch 'envoy/master' into filter

9554677

aunu53 force-pushed the filter branch from d59825e to 9554677 Compare July 3, 2019 19:43

missed some stuff

7725ec7

Signed-off-by: Auni Ahsan <auni@google.com>

fix compile

c6894b2

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 force-pushed the filter branch from 0225566 to c6894b2 Compare July 8, 2019 13:50

Auni Ahsan added 3 commits July 8, 2019 21:45

Build cleanup

f3d59ce

Signed-off-by: Auni Ahsan <auni@google.com>

missing dep

5747353

Signed-off-by: Auni Ahsan <auni@google.com>

Give up on cleaner dependency tree

48461b9

Signed-off-by: Auni Ahsan <auni@google.com>

zuercher reviewed Jul 9, 2019

View reviewed changes

include/envoy/access_log/access_log.h Outdated Show resolved Hide resolved

source/extensions/filters/http/tap/tap_filter.h Outdated Show resolved Hide resolved

source/extensions/access_loggers/common/access_log_base.h Show resolved Hide resolved

Auni Ahsan added 3 commits July 10, 2019 13:45

Change emitLog to references

cb26b11

Signed-off-by: Auni Ahsan <auni@google.com>

Fix tap filter include

2edc0ff

Signed-off-by: Auni Ahsan <auni@google.com>

fix format

a67413a

Signed-off-by: Auni Ahsan <auni@google.com>

zuercher previously approved these changes Jul 10, 2019

View reviewed changes

zuercher changed the title ~~(access log) Add AccessLog::InstanceImpl base class to handle AccessLog::Filter evaluation~~ access log: Add AccessLoggers::Common::ImplBase base class to handle AccessLog::Filter evaluation Jul 10, 2019

jmarantz self-assigned this Jul 10, 2019

jmarantz reviewed Jul 10, 2019

View reviewed changes

PR changes

3f3eee9

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 dismissed zuercher’s stale review via 3f3eee9 July 11, 2019 19:24

Auni Ahsan added 2 commits July 11, 2019 19:40

Merge remote-tracking branch 'envoy/master' into filter

d7aba77

Signed-off-by: Auni Ahsan <auni@google.com>

Undo function comment change

9e6c52d

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz reviewed Jul 11, 2019

View reviewed changes

Move to lazy singleton

20f7630

Signed-off-by: Auni Ahsan <auni@google.com>

jmarantz approved these changes Jul 16, 2019

View reviewed changes

zuercher approved these changes Jul 16, 2019

View reviewed changes

zuercher merged commit c53b12d into envoyproxy:master Jul 16, 2019

aunu53 deleted the filter branch July 17, 2019 14:29

Conversation

aunu53 commented Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aunu53 commented Jun 28, 2019

Uh oh!

zuercher commented Jun 29, 2019

Uh oh!

zuercher commented Jul 1, 2019

Uh oh!

aunu53 commented Jul 2, 2019

Uh oh!

aunu53 commented Jul 3, 2019

Uh oh!

aunu53 commented Jul 8, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz left a comment • 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!

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!

Choose a reason for hiding this comment

Uh oh!

aunu53 commented Jul 11, 2019

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!

aunu53 Jul 15, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aunu53 commented Jun 21, 2019 •

edited

Loading

jmarantz left a comment •

edited

Loading

aunu53 Jul 15, 2019 •

edited

Loading