access log: Add AccessLoggers::Common::ImplBase base class to handle AccessLog::Filter evaluation #7359

auni53 · 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 [email protected]

Signed-off-by: Auni Ahsan <[email protected]>

include/envoy/access_log/access_log.h

…aderMap& Signed-off-by: Auni Ahsan <[email protected]>

Signed-off-by: Auni Ahsan <[email protected]>

zuercher

I think this is getting pretty close.

include/envoy/access_log/access_log.h

source/common/access_log/access_log_impl.h

auni53 · 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 <[email protected]>

auni53 · 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 <[email protected]>

auni53 · 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 <[email protected]>

auni53 · 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 <[email protected]>

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 <[email protected]>

auni53 · 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 <[email protected]>

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

source/common/access_log/BUILD

@@ -61,3 +34,30 @@ envoy_cc_library(
        "@envoy_api//envoy/config/filter/accesslog/v2:accesslog_cc",
    ],
 )
+
+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 <[email protected]>

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

source/extensions/access_loggers/common/access_log_base.cc

@@ -10,15 +11,15 @@ namespace Common {
 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;
+  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

auni53 added 2 commits June 22, 2019 14:39

Add AccessLog::maybeLog

596e20f

Signed-off-by: Auni Ahsan <[email protected]>

add braces

dd5bb0d

Signed-off-by: Auni Ahsan <[email protected]>

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 <[email protected]>

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

auni53 added 7 commits June 25, 2019 09:28

fix clang-tidy

163dfcb

Signed-off-by: Auni Ahsan <[email protected]>

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

1159b63

Signed-off-by: Auni Ahsan <[email protected]>

Introduce AccessLog::InstanceImpl

9ced41e

Signed-off-by: Auni Ahsan <[email protected]>

Fix format

37c3438

Signed-off-by: Auni Ahsan <[email protected]>

Fix compile error

2580ec2

Signed-off-by: Auni Ahsan <[email protected]>

Fix tap filter

8035a14

Signed-off-by: Auni Ahsan <[email protected]>

fix mock references

d6b107a

Signed-off-by: Auni Ahsan <[email protected]>

auni53 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

auni53 added 2 commits July 2, 2019 08:50

PR changes

afed2b5

Signed-off-by: Auni Ahsan <[email protected]>

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

2673d0e

Signed-off-by: Auni Ahsan <[email protected]>

auni53 added 5 commits July 3, 2019 19:33

add code owner

00c9e6d

Signed-off-by: Auni Ahsan <[email protected]>

change some signatures

3eca1cf

Signed-off-by: Auni Ahsan <[email protected]>

fix more interface

6764d79

Signed-off-by: Auni Ahsan <[email protected]>

final PR changes

33dc965

Signed-off-by: Auni Ahsan <[email protected]>

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

9554677

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

missed some stuff

7725ec7

Signed-off-by: Auni Ahsan <[email protected]>

fix compile

c6894b2

Signed-off-by: Auni Ahsan <[email protected]>

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

auni53 added 3 commits July 8, 2019 21:45

Build cleanup

f3d59ce

Signed-off-by: Auni Ahsan <[email protected]>

missing dep

5747353

Signed-off-by: Auni Ahsan <[email protected]>

Give up on cleaner dependency tree

48461b9

Signed-off-by: Auni Ahsan <[email protected]>

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

auni53 added 3 commits July 10, 2019 13:45

Change emitLog to references

cb26b11

Signed-off-by: Auni Ahsan <[email protected]>

Fix tap filter include

2edc0ff

Signed-off-by: Auni Ahsan <[email protected]>

fix format

a67413a

Signed-off-by: Auni Ahsan <[email protected]>

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 <[email protected]>

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

auni53 added 2 commits July 11, 2019 19:40

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

d7aba77

Signed-off-by: Auni Ahsan <[email protected]>

Undo function comment change

9e6c52d

Signed-off-by: Auni Ahsan <[email protected]>

jmarantz reviewed Jul 11, 2019

View reviewed changes

Move to lazy singleton

20f7630

Signed-off-by: Auni Ahsan <[email protected]>

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

access log: Add AccessLoggers::Common::ImplBase base class to handle AccessLog::Filter evaluation #7359

access log: Add AccessLoggers::Common::ImplBase base class to handle AccessLog::Filter evaluation #7359

auni53 commented Jun 21, 2019 •

edited

Loading

zuercher left a comment

auni53 commented Jun 28, 2019

zuercher commented Jun 29, 2019

zuercher commented Jul 1, 2019

auni53 commented Jul 2, 2019

auni53 commented Jul 3, 2019

auni53 commented Jul 8, 2019

zuercher left a comment

zuercher left a comment

jmarantz left a comment •

edited

Loading

jmarantz Jul 10, 2019

auni53 Jul 11, 2019

jmarantz Jul 10, 2019

auni53 Jul 11, 2019

jmarantz Jul 10, 2019

auni53 Jul 11, 2019

jmarantz Jul 10, 2019

auni53 Jul 11, 2019

auni53 commented Jul 11, 2019

jmarantz Jul 11, 2019

auni53 Jul 15, 2019

jmarantz Jul 11, 2019

auni53 Jul 15, 2019

jmarantz Jul 11, 2019

auni53 Jul 15, 2019 •

edited

Loading

jmarantz Jul 16, 2019

access log: Add AccessLoggers::Common::ImplBase base class to handle AccessLog::Filter evaluation #7359

access log: Add AccessLoggers::Common::ImplBase base class to handle AccessLog::Filter evaluation #7359

Conversation

auni53 commented Jun 21, 2019 • edited Loading

zuercher left a comment

Choose a reason for hiding this comment

auni53 commented Jun 28, 2019

zuercher commented Jun 29, 2019

zuercher commented Jul 1, 2019

auni53 commented Jul 2, 2019

auni53 commented Jul 3, 2019

auni53 commented Jul 8, 2019

zuercher left a comment

Choose a reason for hiding this comment

zuercher left a comment

Choose a reason for hiding this comment

jmarantz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auni53 commented Jul 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auni53 Jul 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auni53 commented Jun 21, 2019 •

edited

Loading

jmarantz left a comment •

edited

Loading

auni53 Jul 15, 2019 •

edited

Loading