tracing: Support dynamically loading tracing libraries. #386

rnburn · 2018-01-03T15:08:36Z

Add options for specifying a tracing library to load dynamically (See also envoyproxy/envoy#2252.)

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

htuch · 2018-01-03T15:26:36Z

api/trace.proto

+  string library = 1 [(validate.rules).string.min_bytes = 1];
+
+  // File containing the configuration for the tracing library.
+  string config_file = 2 [(validate.rules).string.min_bytes = 1];


Might this be more useful operationally if it was closer to DataSource (https://github.com/envoyproxy/data-plane-api/blob/master/api/sds.proto#L27), as it would avoid having to do filesystem management? Envoy could write files out to the filesystem if need be, i.e. if we can't load directly from memory into the trace driver.

+1 we should allow inline config. Can we just use Struct here and use JSON, etc. for this? Or is that too restrictive. At minimum agreed we should use Datasource to allow inline configuration.

Switched to use an inline configuration. (Looks like we'll be ok with having tracers use JSON for dynamic configurations.)

htuch · 2018-01-03T15:28:20Z

docs/root/api-v1/tracing.rst

+.. code-block:: json
+
+  {
+    "type": "dynamic",


Do we need v1 support? Unless there is a strong need for this at your end or by folks in the community, we're trying to feature freeze v1 and encourage new features to be v2 first.

+1 if possible can we drop this.

I removed the v1 support.

mattklein123

nice, small comments

mattklein123 · 2018-01-03T17:43:10Z

api/trace.proto

+  string library = 1 [(validate.rules).string.min_bytes = 1];
+
+  // File containing the configuration for the tracing library.
+  string config_file = 2 [(validate.rules).string.min_bytes = 1];


+1 we should allow inline config. Can we just use Struct here and use JSON, etc. for this? Or is that too restrictive. At minimum agreed we should use Datasource to allow inline configuration.

mattklein123 · 2018-01-03T17:44:16Z

api/trace.proto

@@ -52,3 +53,12 @@ message ZipkinConfig {
  // /api/v1/spans, which is the default value.
  string collector_endpoint = 2 [(validate.rules).string.min_bytes = 1];
 }
+
+message DynamicConfig {


Can we call this DynamicOtConfig? I think it's unlikely but it's possible we would eventually support loadable modules of some other type. Same for above perhaps envoy.dynamic.ot for the type?

mattklein123 · 2018-01-03T17:44:32Z

docs/root/api-v1/tracing.rst

+.. code-block:: json
+
+  {
+    "type": "dynamic",


+1 if possible can we drop this.

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

htuch · 2018-01-10T18:57:38Z

@rnburn friendly ping.

rnburn · 2018-01-10T19:05:40Z

Sorry, I'm trying to get agreement on using json/yaml for tracer configurations so that it could be embedded in the way you suggested.

I started a discussion opentracing/opentracing-cpp#45 (comment) but there were a few concerns on how best to proceed.

Will try to get an update soon.

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

htuch · 2018-01-17T21:47:29Z

@rnburn friendly ping :)

tedsuo · 2018-01-17T23:40:18Z

@htuch @mattklein123 FYI we need to decide on the API between the application and the linked tracer - a serialized protocol like JSON or a more limited interface. Perhaps you'd like to weigh in? My impression is that JSON would be easier for everyone involved, but @yurishkuro needs convincing.

mattklein123 · 2018-01-17T23:46:01Z

IMO JSON is the way to go. Generic and easy.

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

mattklein123

LGTM, small comments.

mattklein123 · 2018-01-22T18:39:19Z

api/trace.proto

-    // supported HTTP trace driver. *envoy.lightstep* and *envoy.zipkin* are
-    // built-in trace drivers.
+    // supported HTTP trace driver. *envoy.lightstep*, *envoy.zipkin*, and
+    // *envoy.dynamic* are built-in trace drivers.


envoy.dynamic.ot?

mattklein123 · 2018-01-22T18:39:46Z

api/trace.proto

@@ -52,3 +53,13 @@ message ZipkinConfig {
  // /api/v1/spans, which is the default value.
  string collector_endpoint = 2 [(validate.rules).string.min_bytes = 1];
 }
+
+message DynamicOtConfig {


nit: Can you add a small message level description?

mattklein123 · 2018-01-22T18:40:14Z

api/trace.proto

+message DynamicOtConfig {
+  // Dynamic library implementing the `OpenTracing API
+  // <https://github.com/opentracing/opentracing-cpp>`_.
+  string library = 1 [(validate.rules).string.min_bytes = 1];


Should this be library_path ? @wora ? Not sure what the standard is here.

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

This PR adds support for dynamically loading tracers into Envoy. It will allow authors of OpenTracing compliant tracers to support envoy without requiring envoy to bake their tracers into its build. Tracing libraries can be enabled by adding a section like this to envoy's configuration: tracing: http: name: envoy.dynamic.ot config: library: /path/to/tracing/library config: { .... } With this change, Envoy will be able to use Jaeger's new C++ tracing library. Some features this will allow that aren't available when using Jaeger as a collector for Zipkin: Propagation will work correctly with other services using Jaeger without requiring them to make configuration changes. Jaeger's different sampling strategies can be used. More efficient tranports are available. While it might also make sense sometime in the future to offer a version of Jaeger or other tracers built into Envoy, decoupled plug-in support will allow for quicker on-boarding of tracers and more experimentation in the area without making Envoy's external dependencies become unmanageable. Notes: One challenge with using dynamically loaded libraries in Envoy is that the standard c++ library is statically compiled in with the option -static-libstdc++. If plugin libraries are linked to the libstdc++ in a standard way, this will create problems as two versions of the standard library will be loaded and calls to their functions from the plugin will be intermingled between them, breaking many of the standard library functions. The approach I took with Jaeger was to have it also use -static-libstdc++ and then use an export map to ensure that it's version of the standard library is used without any intermingling (see dsohowto for background on the approach). It does mean that there will be multiple copies of libstdc++ running, but from what I've heard this can be made to work. Another approach could be to add a config option to link libstdc++ dynamically. This depends on opentracing/opentracing-cpp#45 being merged in to opentracing-cpp, but I wanted to get a PR in early to get feedback on the approach in case we need to make changes to that PR. Risk Level: Medium Testing: I have a small example that demoes this functionality using docker and a branch of jaeger. I also plan to add unit test coverage using a mock tracer library that will be added to opentracing-cpp. Docs Changes: envoyproxy/data-plane-api#386 Release Notes: Included in PR. Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

rnburn added 4 commits January 3, 2018 09:33

Add options for dynamic OpenTracing driver.

379ef19

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

Update tracing config comments.

6b8c546

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

Add v1 documentation for the dynamic tracing driver.

d3f97d5

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

Fix external links.

450af70

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

rnburn mentioned this pull request Jan 3, 2018

tracing: Support dynamically loading tracing libraries. envoyproxy/envoy#2252

Merged

htuch reviewed Jan 3, 2018

View reviewed changes

mattklein123 reviewed Jan 3, 2018

View reviewed changes

Remove v1 support.

bcc9951

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

rnburn mentioned this pull request Jan 9, 2018

Add support for dynamic loading. opentracing/opentracing-cpp#45

Merged

s/DynamicConfig/DynamicOtConfig/

0639529

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

Use inline config.

87b1e88

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

mattklein123 reviewed Jan 22, 2018

View reviewed changes

rnburn added 2 commits January 23, 2018 11:51

s/envoy.dynamic/envoy.dynamic.ot/

4ce80d6

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

Document DynamicOtConfig.

c59f3a9

Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

htuch approved these changes Jan 23, 2018

View reviewed changes

htuch merged commit bbac157 into envoyproxy:master Jan 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: Support dynamically loading tracing libraries. #386

tracing: Support dynamically loading tracing libraries. #386

rnburn commented Jan 3, 2018

htuch Jan 3, 2018

mattklein123 Jan 3, 2018

rnburn Jan 22, 2018

htuch Jan 3, 2018

mattklein123 Jan 3, 2018

rnburn Jan 9, 2018

mattklein123 left a comment

mattklein123 Jan 3, 2018

mattklein123 Jan 3, 2018

mattklein123 Jan 3, 2018

htuch commented Jan 10, 2018

rnburn commented Jan 10, 2018

htuch commented Jan 17, 2018

tedsuo commented Jan 17, 2018

mattklein123 commented Jan 17, 2018

mattklein123 left a comment

mattklein123 Jan 22, 2018

rnburn Jan 23, 2018

mattklein123 Jan 22, 2018

rnburn Jan 23, 2018

mattklein123 Jan 22, 2018

tracing: Support dynamically loading tracing libraries. #386

tracing: Support dynamically loading tracing libraries. #386

Conversation

rnburn commented Jan 3, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Jan 10, 2018

rnburn commented Jan 10, 2018

htuch commented Jan 17, 2018

tedsuo commented Jan 17, 2018

mattklein123 commented Jan 17, 2018

mattklein123 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