tracing: Configure tracer from v2 proto configuration by objectiser · Pull Request #4518 · envoyproxy/envoy

objectiser · 2018-09-24T20:14:03Z

Description:
Use v2 proto config to configure the tracer.

Risk Level: medium

Testing:
Updated the unit tests to use yaml and the v2 proto model.

objectiser · 2018-09-25T10:06:54Z

source/extensions/tracers/zipkin/zipkin_tracer_impl.cc

Wondering whether this field should be changed to BoolValue wrapper? The default is currently false, but in the future we may want to change that to true as 128bit ids are becoming more common.

It might not worth, changing bool to BoolValue is a backward incompatible change so you'll need to follow deprecation process, changing default to true is another breaking change I guess?

Agree - so will leave for now and wait for it to be raised as an issue.

lizan · 2018-09-25T16:54:18Z

source/extensions/tracers/lightstep/config.cc

Can you factor out this to Server::Configuration and let the interface have createEmptyProto as well, just like other extensions? This will make extension interface stable and easier for #4475.

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2018-09-28T11:40:32Z

@lizan Fix applied - not sure why the ipv6_tests failed - do you have permissions to rerun?

lizan

Great, a few nits.

lizan · 2018-09-28T17:34:49Z

source/extensions/tracers/dynamic_ot/config.h

                                          Server::Instance& server) override;
+
+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr{new envoy::config::trace::v2::DynamicOtConfig()};


nit: std::make_unique

lizan · 2018-09-28T17:35:31Z

source/extensions/tracers/lightstep/config.cc

-  Tracing::DriverPtr lightstep_driver{new LightStepDriver{
-      json_config, server.clusterManager(), server.stats(), server.threadLocal(), server.runtime(),
-      std::move(opts), Common::Ot::OpenTracingDriver::PropagationMode::TracerNative}};
+  Tracing::DriverPtr lightstep_driver{


std::make_unique

Sorry missed that.

lizan · 2018-09-28T17:35:41Z

source/extensions/tracers/lightstep/config.h

                                          Server::Instance& server) override;

+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr{new envoy::config::trace::v2::LightstepConfig()};


lizan · 2018-09-28T17:37:11Z

source/extensions/tracers/zipkin/zipkin_tracer_impl.cc

It might not worth, changing bool to BoolValue is a backward incompatible change so you'll need to follow deprecation process, changing default to true is another breaking change I guess?

lizan · 2018-09-28T17:42:28Z

source/extensions/tracers/dynamic_ot/config.cc

+      dynamic_cast<const envoy::config::trace::v2::DynamicOtConfig&>(*config_ptr);
+
+  const std::string library = dynaot_config.library();
+  Json::ObjectSharedPtr json_config = MessageUtil::getJsonObjectFromMessage(dynaot_config.config());


use MessageUtil::getJsonStringFromMessage

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2018-09-29T12:22:04Z

@lizan Test failed due to "No space left on device"

lizan · 2018-10-03T08:44:42Z

source/extensions/tracers/lightstep/config.cc

-  Tracing::DriverPtr lightstep_driver{new LightStepDriver{
-      json_config, server.clusterManager(), server.stats(), server.threadLocal(), server.runtime(),
-      std::move(opts), Common::Ot::OpenTracingDriver::PropagationMode::TracerNative}};
+  Tracing::DriverPtr lightstep_driver{


lizan · 2018-10-03T08:45:13Z

source/extensions/tracers/lightstep/config.h

                                          Server::Instance& server) override;

+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr{std::make_unique<envoy::config::trace::v2::LightstepConfig>()};


I think you can just return std::make_unique<...>(); without wrapping into MessagePtr

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser · 2018-10-03T11:51:37Z

@lizan hopefully all sorted now.

mattklein123

Nice, thanks for cleaning this up.

Signed-off-by: Gary Brown <gary@brownuk.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>

objectiser force-pushed the tracingconfig branch 2 times, most recently from ffe6c4b to 079a3c5 Compare September 24, 2018 20:20

objectiser commented Sep 25, 2018

View reviewed changes

lizan suggested changes Sep 25, 2018

View reviewed changes

mattklein123 assigned lizan Sep 25, 2018

objectiser added 2 commits September 28, 2018 10:37

tracing: Configure tracer from v2 proto configuration

4e2635e

Signed-off-by: Gary Brown <gary@brownuk.com>

Factor out creation of empty proto message

2a8658e

Signed-off-by: Gary Brown <gary@brownuk.com>

objectiser force-pushed the tracingconfig branch from 079a3c5 to 2a8658e Compare September 28, 2018 10:49

lizan suggested changes Sep 28, 2018

View reviewed changes

Fix additional comments

2c1f825

Signed-off-by: Gary Brown <gary@brownuk.com>

lizan reviewed Oct 3, 2018

View reviewed changes

Changes based on further comments

d184823

Signed-off-by: Gary Brown <gary@brownuk.com>

lizan approved these changes Oct 3, 2018

View reviewed changes

lizan requested a review from htuch October 3, 2018 16:56

mattklein123 approved these changes Oct 3, 2018

View reviewed changes

mattklein123 merged commit 38a8be2 into envoyproxy:master Oct 3, 2018

aa-stripe pushed a commit to aa-stripe/envoy that referenced this pull request Oct 11, 2018

tracing: Configure tracer from v2 proto configuration (envoyproxy#4518)

b05acc8

Signed-off-by: Gary Brown <gary@brownuk.com> Signed-off-by: Aaltan Ahmad <aa@stripe.com>

Conversation

objectiser commented Sep 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

objectiser commented Sep 28, 2018

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

objectiser commented Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

objectiser commented Oct 3, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

objectiser commented Sep 24, 2018 •

edited

Loading

objectiser commented Sep 29, 2018 •

edited

Loading