Add static registration of AccessLog::Instance implementations. by mrice32 · Pull Request #1609 · envoyproxy/envoy

mrice32 · 2017-09-07T19:47:13Z

This enables custom access log implementations to be substituted in place of the existing file access log. This feature is only available in the v2 api. For v1 api users, there should be no changes. This change increments the commit hash of envoy-api to get related api changes.

The implementation follows the static registration pattern for other components relatively closely.

htuch

Looks good, some code structure questions..

htuch · 2017-09-08T02:03:33Z

source/common/config/filter_json.cc

+
+  // Statically registered access logs are a v2-only feature, so use the standard internal file
+  // access log for json config conversion.
+  access_log.set_name("envoy.file_access_log");


We should probably have a centralized list of well known names in the Envoy namespace. I started one for metadata in source/common/config/metadata.h, but we should probably expand to cover this.

htuch · 2017-09-08T02:05:16Z

source/common/http/access_log/access_log_impl.cc

+  ProtobufTypes::MessagePtr message = factory->createEmptyConfigProto();
+  if (config.has_config()) {
+    MessageUtil::jsonConvert(config.config(), *message);
+  }


This pattern is now in at least 4 places. What do you think about refactoring?

htuch · 2017-09-08T02:07:02Z

include/envoy/http/access_log.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
+   * was specified in the configuration, argument will be nullptr.
+   * @param log_manager interface through which filesystem access logs are created.


Is this the only way to plumb this for the filesystem logs? Seems like a vestigial feature for everyone else.

We could do something more general, like provide an "environment" or "context" to give general access to conditionally useful variables so that this api can be expanded with new use cases (for instance, we will likely need to provide the ability to access the cluster manager for RPC logs) as is done for http filters now. We could also make the log_manager a singleton, but that seems like it would be more troublesome for each additional object.

+1 I think a context is good, it leaves room for future expansion without having to modify the filter interface, which causes breaking/deprecating changes.

mrice32 · 2017-09-09T00:56:02Z

Added singleton of well known names and replaced uses with references to the class, used the FactoryContext object to provide a context for the AccessLogInstanceFactory, and generalized the factory lookup and config conversion.

mattklein123

looks great. small doc nit.

mattklein123 · 2017-09-10T15:14:11Z

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
+   * was specified in the configuration, argument will be nullptr.
+   * @param log_manager interface through which filesystem access logs are created.


nit: doc out of date.

htuch · 2017-09-11T14:34:27Z

source/common/config/well_known_names.h

+class NetworkFilterNameValues {
+public:
+  // Client ssl auth filter
+  const std::string CLIENT_SSL_AUTH = "client_ssl_auth";


I'm wondering for the network/HTTP names whether we want to make it that we retain existing names without a envoy. prefix for v1, but for v2 make these envoy.ratelimit etc. We could just do the prefix step during the JSON -> proto translation steps and then we have a consistent naming scheme across all filters/pluggables for v2.

htuch · 2017-09-11T14:35:38Z

source/common/config/well_known_names.h

@@ -0,0 +1,96 @@
+#pragma once


Thanks for adding this. Can you merge https://github.com/lyft/envoy/blob/master/source/common/config/metadata.h#L43 into this as well?

mattklein123 · 2017-09-11T19:30:50Z

source/common/config/well_known_names.h

+  const std::string ROUTER = "envoy.router";
+
+  HttpFilterNameValues() {
+    const std::vector<std::string> prefixed_v2_names = {


This thing seems fragile to me but I can't really think of a better way to do it. Can we potentially at least share this code with above via some small helper class that takes an array of strings as part of the constructor?

Yeah, my bad. Should've been done that way in the first place.

mattklein123

Nice LGTM pending @htuch final look.

htuch

Thanks for the epic cleanup on filter namespace. A very minor nit (ENVOY_ROUTER redundancy), otherwise LGTM.

htuch · 2017-09-12T01:45:13Z

configs/google_com_proxy.v2.yaml

                route: { host_rewrite: www.google.com, cluster: service_google }
          http_filters:
-          - name: router
+          - name: envoy.router


Really appreciate the cleanup here! :)

htuch · 2017-09-12T01:48:24Z

source/common/config/well_known_names.h

+        throw EnvoyException(fmt::format(
+            "Attempted to create a conversion for a v2 name that isn't prefixed by {}", prefix));
+      }
+      v1_to_v2_names_[name.substr(prefix.size())] = name;


I think this could be done a bit simpler by just prefixing on ingest, but it seems alright, and will eventually be deprecated.

htuch · 2017-09-12T01:49:54Z

source/common/config/well_known_names.h

+  // Filter namespace for built-in load balancer.
+  const std::string ENVOY_LB = "envoy.lb";
+  // Filter namespace for built-in router opaque data.
+  const std::string ENVOY_ROUTER = "envoy.router";


I think ENVOY_ROUTER is a dupe of ROUTER above. The idea in the metadata namespace is you use the filter name as key.

Does this mean that these should all just be treated as filter names and we should remove this class altogether?

Description: Remove the designated initializer I used in `engine_builder.cc` and instead populate `null_logger` field-by-field. Turns out this syntax is one of this parts of C++ that's _not_ the same as C. Risk Level: Low Testing: `bazel test //test/...` Docs Changes: N/A Release Notes: N/A Signed-off-by: Cerek Hillen <chillen@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Added static registration of AccessLogInstance implementations.

6f0c1fc

htuch reviewed Sep 8, 2017

View reviewed changes

added context object, well known names, and abstracted lookup

87e166f

removed extra includes

9f7e2f8

mattklein123 reviewed Sep 10, 2017

View reviewed changes

corrected outdated doc

af80068

htuch reviewed Sep 11, 2017

View reviewed changes

mrice32 added 2 commits September 11, 2017 15:18

updated names of filters and merged metadata names into wkn file

a87b7f1

fix format

96a6087

mattklein123 reviewed Sep 11, 2017

View reviewed changes

mrice32 added 3 commits September 11, 2017 16:32

moved name converter to standalone class

bfbca12

doc correction

6e3cb72

Merge branch 'master' into access_log_registration

0b8ee27

htuch mentioned this pull request Sep 11, 2017

config: ADS implementation. #1621

Closed

fixed integration test post merge

2997044

mattklein123 previously approved these changes Sep 12, 2017

View reviewed changes

htuch previously approved these changes Sep 12, 2017

View reviewed changes

deduped router name

95118bb

mrice32 dismissed stale reviews from htuch and mattklein123 via 95118bb September 12, 2017 14:21

mattklein123 mentioned this pull request Sep 12, 2017

Add CORS filter #1528

Merged

mattklein123 approved these changes Sep 12, 2017

View reviewed changes

htuch approved these changes Sep 12, 2017

View reviewed changes

htuch merged commit be01fc2 into envoyproxy:master Sep 12, 2017

mrice32 deleted the access_log_registration branch September 12, 2017 16:20

mathetake added a commit that referenced this pull request Mar 3, 2026

refactor: removes unnecessary code for /models (#1609)

23779b7

Conversation

mrice32 commented Sep 7, 2017

Uh oh!

htuch 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!

mrice32 Sep 8, 2017 • 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!

mrice32 commented Sep 9, 2017

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!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

htuch 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!

mrice32 Sep 12, 2017 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mrice32 Sep 8, 2017 •

edited

Loading

mrice32 Sep 12, 2017 •

edited

Loading