Added static registration for http tracers. (#967) #994

mrice32 · 2017-05-18T19:51:38Z

Added a static registration mechanism (following the pattern of filters mentioned in the comments on #967) for http tracers. This will likely be expanded to a more general registration mechanism in future work. This PR is part of the work towards solving #967.

mattklein123 · 2017-05-18T19:55:38Z

I think @htuch will take first pass, but meta comment, if this goes in I think #982 can lose the defines and just not compile lightstep code. I think this will also solve @lizan issue.

jamessynge · 2017-05-18T20:01:53Z

source/server/config/http/lightstep_http_tracer.cc

+
+  Envoy::Runtime::RandomGenerator& rand = server.random();
+
+  std::unique_ptr<lightstep::TracerOptions> opts(new lightstep::TracerOptions());


Please include lightstep/options.h and lightstep/tracer.h

jamessynge · 2017-05-18T20:14:59Z

source/server/configuration_impl.h

+ * Implemented by each network filter and registered via registerHttpTracerFactory() or
+ * the convenience class RegisterHttpTracerFactory.
+ */
+class HttpTracerFactory {


Is there a good reason to NOT put this in include/envoy/tracing/http_tracer.h?

It creates a build dependency cycle because //include/envoy/tracing:http_tracer_interface and //include/envoy/server:instance_interface would both depend on one another.

jamessynge · 2017-05-18T20:16:04Z

source/server/configuration_impl.h

@@ -44,6 +45,19 @@ class NetworkFilterConfigFactory {
 };

 /**
+ * Implemented by each network filter and registered via registerHttpTracerFactory() or


Implemented for each kind of HttpTracer.

jamessynge · 2017-05-18T20:16:31Z

source/server/configuration_impl.h

+public:
+  virtual ~HttpTracerFactory() {}
+
+  virtual Tracing::HttpTracerPtr


Please document the method (expected behavior, error conditions).

jamessynge · 2017-05-18T20:19:52Z

source/server/configuration_impl.cc

+    if (http_tracer) {
+      http_tracer_ = std::move(http_tracer);
+      found_tracer = true;
+    }


mattklein123

lgtm, nice work. Small nit, will let others follow up with other comments/approve.

mattklein123 · 2017-05-18T22:36:34Z

source/server/configuration_impl.h

@@ -135,6 +159,11 @@ class MainImpl : Logger::Loggable<Logger::Id::config>, public Main {
    return filter_config_factories;
  }

+  static std::list<HttpTracerFactory*>& httpTracerFactories() {
+    static std::list<HttpTracerFactory*> http_tracer_factories;


If we are following the letter of the law here I think this should be a pointer and the list allocated with new

mattklein123 · 2017-05-18T22:48:01Z

@mrice32 also please sign the CLA

htuch

LGTM, thanks for adding this!

htuch · 2017-05-19T10:29:14Z

source/server/configuration_impl.h

+template <class T> class RegisterHttpTracerFactory {
+public:
+  RegisterHttpTracerFactory() {
+    static T instance;


In addition to the above static std::list, this should also be created on use. For reference:

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables
https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use

htuch · 2017-05-19T10:30:47Z

source/server/config/http/zipkin_http_tracer.h

+class ZipkinHttpTracerFactory : public HttpTracerFactory {
+public:
+  // HttpTracerFactory
+  Tracing::HttpTracerPtr tryCreateHttpTracer(const std::string& type,


I'm a bit surprised by the interface (although it's consistent with the existing factory interface for filters). How come we need to probe each individual filter/tracer to figure out if they match type, rather than registering a map from type to filter/tracer? @mattklein123 for the filter interface.

We should do that. :) I have no idea why I did it this way in the first place. Very long time ago now. Feel free to fix in this PR or a follow up.

Okay, cool. I'll add it into this PR.

This has the added benefit of you can fail if you try to register something with the same name. So would be great to add test for that also, and fix all the networking filters too. Bigger change, but I think well worth it.

mrice32 · 2017-05-19T21:38:18Z

Took a stab at the map static registration mechanism, PTAL.

ccaraman · 2017-05-22T18:18:48Z

CLA has been signed.

RomanDzhabarov · 2017-05-22T19:11:06Z

test/server/config/network/config_test.cc

               Json::Exception);
 }

+TEST(NetworkFilterConfigTest, DoubleRegistrationTest) {
+  EXPECT_THROW(RegisterNetworkFilterConfigFactory<ClientSslAuthConfigFactory>(), EnvoyException);


nit: I think EXPECT_THROW_WITH_MESSAGE makes a test slightly better (that macro is from common_test/utility.h)
In Envoy we throw EnvoyExceptions for various reasons it's better to verify that it's what we expect.

RomanDzhabarov · 2017-05-22T19:18:01Z

source/server/configuration_impl.h

-                                                        const Json::Object& config,
-                                                        Server::Instance& server) PURE;
+  /**
+  * Create a particular network filter factory implementation.  If the creation fails, an error will


can you be more specific, is it only EnvoyException which can be thrown?

RomanDzhabarov · 2017-05-22T19:18:45Z

source/server/configuration_impl.h

+  * Create a particular network filter factory implementation.  If the creation fails, an error will
+  * be thrown.  The returned callback should always be valid.
+  */
+  virtual NetworkFilterFactoryCb createFilterFactory(NetworkFilterType type,


i think we are a bit more strict now around documenting @params etc, can you add those

RomanDzhabarov · 2017-05-22T19:30:04Z

source/server/configuration_impl.h

-    static std::list<NetworkFilterConfigFactory*> filter_config_factories;
-    return filter_config_factories;
+  static std::unordered_map<std::string, NetworkFilterConfigFactory*>& filterConfigFactories() {
+    static std::unordered_map<std::string, NetworkFilterConfigFactory*>* filter_config_factories =


any cons that to be: static std::unordered_map<std::string, NetworkFilterConfigFactory*> filter_config_factories; ?

Yeah, the static initialization order fiasco :) See https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use. We have a section on this now in https://github.com/lyft/envoy/blob/master/STYLE.md.

oki doki, i have not inspected all the code, but looks like https://github.com/lyft/envoy/blob/master/source/server/config/network/http_connection_manager.h#L117 has a similar issue as you pointed out @htuch

RomanDzhabarov · 2017-05-22T19:34:21Z

source/server/config/network/http_connection_manager.cc

-      throw EnvoyException(fmt::format("unable to create http filter factory for '{}'/'{}'",
-                                       string_name, string_type));
-    }
+    HttpFilterConfigFactory* factory = filterConfigFactories().at(string_name);


do we have a test when we call it without "string_name" filtered registered

RomanDzhabarov · 2017-05-22T19:38:12Z

source/server/config/network/http_connection_manager.h

+  * Provides the identifying name for a particular implementation of an http filter produced by the
+  * factory.
+  */
+  virtual std::string name() PURE;


nbd (as there are calls to it only on a startup)
but this could be const std::string&

I was curious about this. We would need to define a string as a member of the class or make a static string, right? If I'm thinking about it correctly, both of those options would ultimately be more copying than the current version (because we should only call the method once during registration and the string produced will be moved into the unordered_map as opposed to the copy required for a const std::string &). However, if this is just a readability/API preference for envoy, that totally makes sense.

yeah, we could do static const string in a class.
Thinking about this more, as it's called once there is no point of having ref returned, agreed.

…responded to comments

htuch

LGTM after @RomanDzhabarov comments are taken care of. I think you can easily add a shim to support the legacy filter API. The filters that you've converted here are good for the new API, but it would make sense to have a test for at least one dummy filter (just duplicate the Echo filter maybe) that conforms to the deprecated API to ensure we don't regress there until 1.4.0.

htuch · 2017-05-23T05:36:04Z

source/server/config/http/dynamo.cc

+                                                            const Json::Object&,
+                                                            const std::string& stat_prefix,
+                                                            Server::Instance& server) {
+  if (type != HttpFilterType::Both) {


Unrelated to the review, a question that came up recently while discussing filters with @alyssawilk. Why is Envoy filter config structured such that (1) filters know their HttpFilterType yet (2) the type is specified in the config as well, meaning we need to check consistency. It seems this could be elided from the config. The only thing I can think of is that some filters might work as encoder/decoder depending upon configuration... @RomanDzhabarov @ccaraman @junr03 for insight here.

@htuch cannot think about anything else, but fwiw, currently, no HTTP filters support this behavior.

htuch · 2017-05-23T05:40:57Z

source/server/configuration_impl.h

-    static std::list<NetworkFilterConfigFactory*> filter_config_factories;
-    return filter_config_factories;
+  static std::unordered_map<std::string, NetworkFilterConfigFactory*>& filterConfigFactories() {
+    static std::unordered_map<std::string, NetworkFilterConfigFactory*>* filter_config_factories =


Yeah, the static initialization order fiasco :) See https://isocpp.org/wiki/faq/ctors#static-init-order-on-first-use. We have a section on this now in https://github.com/lyft/envoy/blob/master/STYLE.md.

mrice32 · 2017-05-23T14:00:57Z

Thanks @RomanDzhabarov for the thorough review. It's really helpful as I'm trying to learn the how things should be done in envoy. I believe I have corrected for most of the comments except for one that I had a question about. @htuch I have updated to add the backwards compatibility. I documented the list registration related methods/classes as DEPRECATED in the comments. Please let me know if there's some other place I should document the deprecation. I will add a dummy test to ensure that the deprecated filter registration continues to work and commit it today.

RomanDzhabarov · 2017-05-23T19:21:23Z

LGTM, @htuch for final pass

Automatic merge from submit-queue. JWT-auth: Support aud as string array **What this PR does / why we need it**: To support "aud" in JWT as string array. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # **Special notes for your reviewer**: **Release note**: ```release-note None ```

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Added static registration for http tracers. (envoyproxy#967)

485f5d3

jamessynge reviewed May 18, 2017

View reviewed changes

Addresses review comments, PTAL.

597a76f

mattklein123 reviewed May 18, 2017

View reviewed changes

htuch reviewed May 19, 2017

View reviewed changes

mrice32 added 3 commits May 19, 2017 10:26

Corrected static initialization.

4963472

Corrected formatting errors.

402a4e9

Modified static registration to use unordered_map

7f59e9b

htuch mentioned this pull request May 22, 2017

Hack to allow conditionally compiling out lightstep #982

Closed

tschroed mentioned this pull request May 22, 2017

Make zipkin and lightstep optional based on bazel --define #1006

Closed

RomanDzhabarov assigned RomanDzhabarov and goaway May 22, 2017

RomanDzhabarov reviewed May 22, 2017

View reviewed changes

Deprecated and added back list registration behavior for filters and …

050c7d0

…responded to comments

htuch reviewed May 23, 2017

View reviewed changes

mrice32 added 2 commits May 23, 2017 10:22

fixed formatting

1090023

Added tests to ensure deprecated registration style continues to work.

cb97780

RomanDzhabarov approved these changes May 23, 2017

View reviewed changes

RomanDzhabarov mentioned this pull request May 23, 2017

Adds grpc-web support for envoy. #943

Merged

htuch mentioned this pull request May 24, 2017

Create formal deprecation process #1012

Closed

htuch approved these changes May 24, 2017

View reviewed changes

htuch merged commit 3a466c3 into envoyproxy:master May 24, 2017

mrice32 deleted the registration branch May 24, 2017 15:37

mattklein123 unassigned goaway and RomanDzhabarov Sep 14, 2017

jpsim pushed a commit that referenced this pull request Nov 28, 2022

release: v0.3.1.07312020 (#994)

0f1c6b2

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

jpsim pushed a commit that referenced this pull request Nov 29, 2022

release: v0.3.1.07312020 (#994)

e37fac7

Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added static registration for http tracers. (#967) #994

Added static registration for http tracers. (#967) #994

mrice32 commented May 18, 2017

mattklein123 commented May 18, 2017

jamessynge May 18, 2017

jamessynge May 18, 2017

mrice32 May 18, 2017

jamessynge May 18, 2017

jamessynge May 18, 2017

jamessynge May 18, 2017

mattklein123 left a comment

mattklein123 May 18, 2017

mattklein123 commented May 18, 2017

htuch left a comment

htuch May 19, 2017

htuch May 19, 2017

mattklein123 May 19, 2017

mrice32 May 19, 2017

mattklein123 May 19, 2017

mrice32 commented May 19, 2017

ccaraman commented May 22, 2017

RomanDzhabarov May 22, 2017

RomanDzhabarov May 22, 2017

RomanDzhabarov May 22, 2017

RomanDzhabarov May 22, 2017

htuch May 23, 2017

RomanDzhabarov May 23, 2017

RomanDzhabarov May 22, 2017

RomanDzhabarov May 22, 2017

mrice32 May 23, 2017

RomanDzhabarov May 23, 2017

htuch left a comment

htuch May 23, 2017

RomanDzhabarov May 23, 2017

htuch May 23, 2017

mrice32 commented May 23, 2017

RomanDzhabarov commented May 23, 2017


		Envoy::Runtime::RandomGenerator& rand = server.random();

		std::unique_ptr<lightstep::TracerOptions> opts(new lightstep::TracerOptions());

Added static registration for http tracers. (#967) #994

Added static registration for http tracers. (#967) #994

Conversation

mrice32 commented May 18, 2017

mattklein123 commented May 18, 2017

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

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented May 18, 2017

htuch left a 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

mrice32 commented May 19, 2017

ccaraman commented May 22, 2017

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

Choose a reason for hiding this comment

htuch left a 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

mrice32 commented May 23, 2017

RomanDzhabarov commented May 23, 2017