Made stats sinks a statically registered component #1506

mrice32 · 2017-08-21T20:10:50Z

This is part of a larger effort to make various implementations more flexible by allowing proprietary components to be statically registered with Envoy needing only to be linked to the binary at build time and configured at runtime #967 .

The user-visible configuration changes do involve a few deprecations: statsd_udp_ip_address and statsd_tcp_cluster_name will be deprecated, and their equivalents will be moved to a new stats_sinks array where the configurations for any statically registered sink will exist. As a part of this change, all integration tests were moved to the new configuration format, but a new configuration test was added to ensure that the deprecated format continues to work until it is removed in 1.5.0.

mrice32 · 2017-08-21T20:10:57Z

@htuch

htuch

Thanks for adding this. I'm wondering if we want to go straight to v2 though, we're basically ready to do this now by adding these fields into the bootstrap proto.

htuch · 2017-08-21T21:36:32Z

source/common/json/config_schemas.cc

@@ -1187,6 +1187,10 @@ const std::string Json::Schema::TOP_LEVEL_CONFIG_SCHEMA(R"EOF(
      "statsd_udp_ip_address" : {"type" : "string"},
      "statsd_tcp_cluster_name" : {"type" : "string"},
      "stats_flush_interval_ms" : {"type" : "integer"},
+      "stats_sinks" : {


I will be added support to the v2 bootstrap proto (https://github.com/lyft/envoy-api/blob/master/api/bootstrap.proto) to cover this functionality (as well as a bunch of stuff at the top-level) tomorrow. Do you think you want this feature in v1, or maybe it would make sense to make it v2 only?

htuch · 2017-08-21T21:40:36Z

source/server/config/stats/tcp_statsd.h

+class TcpStatsdSinkFactory : Logger::Loggable<Logger::Id::config>, public StatsSinkFactory {
+public:
+  // StatsSinkFactory
+  Stats::SinkPtr createStatsSink(const Json::Object& json_config, Instance& server,


Would prefer we go proto-only on this new feature, see what is being done in https://github.com/lyft/envoy/pull/1495/files.

htuch · 2017-08-21T21:42:29Z

source/server/configuration_impl.cc

+
+  // Used to convert the deprecated statsd configuration into a json snippet for consumption by the
+  // new sink config parsing logic.
+  const std::string statsd_json = R"EOF(


See the translate*() conversion functions in source/common/config/*_json.cc for the stylized way we are mapping from v1 JSON to v2 proto.

htuch · 2017-08-21T21:52:40Z

source/server/config/stats/tcp_statsd.cc

+      fmt::format("Didn't find cluster_name in the {} Stats::Sink config", name()));
+}
+
+std::string TcpStatsdSinkFactory::name() { return "statsd_tcp"; }


Suggest using the reverse DNS scheme described in https://github.com/lyft/envoy-api/blob/master/README.md#principles for custom components. I think for builtins, we would use envoy.statsd_tcp, since I've been using the envoy. prefix other places in v2 for builtins.

mrice32 · 2017-08-21T21:56:36Z

That makes sense. Let's go straight to v2 with this.

mattklein123 · 2017-08-21T21:57:36Z

@mrice32 @htuch do we want to try to land this before 1.4.0 is tagged (probably Friday)? Or can we potentially wait until next week and make this a 1.5.0 deprecation?

mrice32 · 2017-08-21T22:13:25Z

It was intended to be a 1.5.0 deprecation, and there's no rush on getting it merged, so we can definitely wait til next week to keep it from complicating the 1.4.0 release.

mattklein123 · 2017-08-21T22:17:32Z

OK perfect thank you.

mattklein123 · 2017-08-25T02:05:59Z

@mrice32 can you merge master, then we can get this in.

mrice32 · 2017-08-25T13:55:16Z

I think we should wait on #1521 to get merged. Otherwise, we'll have to reimplement the stats portion of the json -> proto conversion from that PR in this one. If you'd like we can close this for now and reopen once that gets merged.

mattklein123 · 2017-08-25T14:51:35Z

OK sounds good. I will review #1521.

mrice32 · 2017-09-01T22:54:28Z

Merged and should be ready for review.

htuch

Looks great.

htuch · 2017-09-03T12:10:16Z

source/server/config/stats/statsd.cc

+
+#include <string>
+
+#include "envoy/registry/registry.h"


Nit: Any header that is depended upon in .{cc,h} should be directly reflected in the deps in the BUILD file. This avoids surprises when A depends on B providing C, but later on, B no longer depends on C. Since we lack tool automation for this in OSS, we don't really enforce it (I'm sure I'm guilty of many breaches of this), but best to try and shoot for it. Basically, you want IWYU (include what you use, i.e. have all include headers in a file directly for symbols that appear in the file) and then BUILD dep pointing at libs providing the headers.

htuch · 2017-09-03T12:15:16Z

source/server/configuration_impl.cc

+
+    if (!sink_object.has_config()) {
+      throw EnvoyException(
+          "sink object does not contain the 'config' object to configure the implementation");


I think an empty default config is legit, we could have some filters that don't require config.

htuch · 2017-09-03T12:18:02Z

source/server/configuration_impl.h

+  /**
+   * Create a particular Stats::Sink implementation. If the implementation is unable to produce a
+   * Stats::Sink with the provided parameters, it should throw an EnvoyException in the case of
+   * general error or a Json::Exception if the json configuration is erroneous. The returned


Why Json::Exception? This is a pure v2 feature, we shouldn't have RapidJSON involved as we're working in proto land here. The only JSON conversion is performed by the protobuf library that won't create these types of exceptions.

Oh, I see, this is copy+paste from https://github.com/lyft/envoy/blob/master/include/envoy/server/filter_config.h. In filter_config.h, the JSON stuff doesn't apply to the ...FromProto() variants.

Right, sorry, that was just a typo. Thought I removed it.

htuch · 2017-09-03T12:19:42Z

source/server/configuration_impl.h

+   * @param cluster_manager supplies the cluster_manager instance
+   */
+  virtual Stats::SinkPtr createStatsSink(const Protobuf::Message& config, Instance& server,
+                                         Upstream::ClusterManager& cluster_manager) PURE;


Why is ClusterManager a distinct parameter here? I think it can be inferred from the Server::Instance parameter?

htuch · 2017-09-03T12:21:57Z

test/server/config/stats/config_test.cc

+using testing::NiceMock;
+using testing::Return;
+using testing::ReturnRef;
+using testing::_;


Nit: place outside the namespace block as per Envoy convention.

There are a few other tests (including the one I modeled after) that put it inside the Envoy namespace block. Should I go ahead and update those as well?

Yep, thanks.

htuch · 2017-09-03T12:23:05Z

test/server/config/stats/config_test.cc

+TEST(StatsConfigTest, ValidTcpStatsd) {
+  const std::string name = "envoy.statsd";
+  Protobuf::Struct config;
+  ProtobufWkt::Map<ProtobufTypes::String, ProtobufWkt::Value>& field_map = *config.mutable_fields();


Prefer using auto here and elsewhere for proto handlers, the extra noise of the complex types doesn't help readability.

htuch · 2017-09-03T12:24:23Z

test/server/config/stats/config_test.cc

+  EXPECT_THROW(factory->createStatsSink(*message, server, server.clusterManager()), EnvoyException);
+}
+
+} // namespace Configuration


There's probably a few of the throw EnvoyException cases that don't have coverage in this file. Can you add?

mrice32 · 2017-09-05T15:51:34Z

Responded to comments. Added tests for other throw cases in the tests for ConfigurationImpl since those throws were in that file.

htuch

LGTM, two small questions..

htuch · 2017-09-05T16:39:54Z

test/server/config/stats/config_test.cc

+      *address_field_map["socket_address"].mutable_struct_value()->mutable_fields();
+  socket_address_field_map["protocol"].set_string_value("UDP");
+  socket_address_field_map["address"].set_string_value("127.0.0.1");
+  socket_address_field_map["port_value"].set_number_value(8125);


Why not build a typed envoy::api::v2::Address and the JsonConvert() it to the Struct? Seems a bit more robust.

Yeah, I had an internal debate about which one was a better way to go about testing it. In hindsight, since we're moving to a protobuf specified bootstrap config, I think your suggestion makes sense. I'll update.

htuch · 2017-09-05T16:41:05Z

test/server/config/stats/config_test.cc

+  EXPECT_NE(dynamic_cast<Stats::Statsd::UdpStatsdSink*>(sink.get()), nullptr);
+}
+
+TEST(StatsConfigTest, EmptyConfig) {


I thought you removed the empty config exception above, how come this still throws?

Because an empty config is invalid for that particular sink (statsd). I can add an expected throw message to clarify.

These tests are not calling into the ConfigurationImpl object. They are meant to test the configuration of specific instances of stats sinks rather than the general configuration mechanism in ConfigurationImpl.

mrice32 · 2017-09-05T18:50:08Z

Hmmm, //test/common/network:connection_impl_test seems to be failing on OS X CI, but I don't think I modified anything that that would depend on...?

htuch · 2017-09-05T18:56:05Z

@mrice32 that's a known flake/failure.

mattklein123

Nice. Thanks for fixing all the namespace stuff!

Description: This PR modifies the build recipe for `envoy_engine.so` to hide all symbols except for `_PyInit_envoy_engine` by default on maOS, so that envoy_engine can be loaded alongside a [protobuf](http://pypi.org/project/protobuf) wheel without breaking. Loading in protobuf alongside `envoy_engine.so` would register two sets of the same symbols and, on macOS, the runtime linker chooses the first symbol it finds causing problems. See the [similar protobuf change](protocolbuffers/protobuf#8346) and its [sister change in grpc](grpc/grpc#24992) for more information on what's going on. Risk Level: Low Testing: See the now-closed #1504 for how I've been testing this. Docs Changes: N/A Release Notes: N/A Signed-off-by: Cerek Hillen <[email protected]> Signed-off-by: JP Simard <[email protected]>

Made Stats Sinks a statically registered component

6c427c6

corrected doc formatting

42ea55a

htuch reviewed Aug 21, 2017

View reviewed changes

mrice32 added 2 commits September 1, 2017 14:52

Merge branch 'master' into sta_reg

cf5fac0

Fixed post-merge issues and simplified PR

f99b5a5

removed debug lines

441b3a7

htuch reviewed Sep 3, 2017

View reviewed changes

Responded to comments and added additional tests

d78014d

htuch reviewed Sep 5, 2017

View reviewed changes

Modified the construction of proto in new tests

9dcc3a5

htuch approved these changes Sep 5, 2017

View reviewed changes

mattklein123 approved these changes Sep 5, 2017

View reviewed changes

htuch merged commit 0702b29 into envoyproxy:master Sep 5, 2017

mrice32 deleted the sta_reg branch October 26, 2017 15:48


		#include <string>

		#include "envoy/registry/registry.h"

Made stats sinks a statically registered component #1506

Made stats sinks a statically registered component #1506

Conversation

mrice32 commented Aug 21, 2017 • edited Loading

mrice32 commented Aug 21, 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

mrice32 commented Aug 21, 2017

mattklein123 commented Aug 21, 2017

mrice32 commented Aug 21, 2017

mattklein123 commented Aug 21, 2017

mattklein123 commented Aug 25, 2017

mrice32 commented Aug 25, 2017 • edited Loading

mattklein123 commented Aug 25, 2017

mrice32 commented Sep 1, 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

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 Sep 5, 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

mrice32 Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

mrice32 Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

mrice32 commented Sep 5, 2017 • edited Loading

htuch commented Sep 5, 2017

mattklein123 left a comment

Choose a reason for hiding this comment

mrice32 commented Aug 21, 2017 •

edited

Loading

mrice32 commented Aug 25, 2017 •

edited

Loading

mrice32 Sep 5, 2017 •

edited

Loading

mrice32 Sep 5, 2017 •

edited

Loading

mrice32 commented Sep 5, 2017 •

edited

Loading