tracing: Support dynamically loading tracing libraries. #2252

rnburn · 2017-12-21T21:50:29Z

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

Description:

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 Add support for dynamic loading. 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>

mattklein123 · 2017-12-22T00:03:42Z

@rnburn this is sweet. Real quick comment (have not reviewed): This is going to need v2 config defined here: https://github.com/envoyproxy/data-plane-api/blob/master/api/trace.proto. (Everything now is v2 first. v1 is optional). Can you do the data-plane-api PR and then work that into this PR?

mattklein123 · 2017-12-22T00:04:34Z

Also, from a quick scan, this will need tests. You are probably going to have to compile some type of test driver as an .so and load it or something. Not sure what is best.

rnburn · 2017-12-22T00:07:18Z

Yes, I'm working on a mocktracer library (similar to what's in opentracing-go) that will be distributed as part of opentracing-cpp and can be used to test dynamic loading against.

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

rnburn · 2018-01-03T15:11:01Z

I added a data plane PR to support corresponding v2 options and to document the v1 options.

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

Add options for specifying a tracing library to load dynamically (See also envoyproxy/envoy#2252.) Signed-off-by: Ryan Burn <ryan.burn@gmail.com>

mattklein123 · 2018-01-25T01:27:53Z

@rnburn friendly ping on this. If this won't be completed for a while do you mind if we close for now and reopen when ready?

rnburn · 2018-01-25T12:48:32Z

Sorry for the delay.

I just added in a mocktracer library implementing the dynamic loading interface to opentracing-cpp (opentracing/opentracing-cpp@359bafe) that I can use to add testing coverage for the dynamic loading code. I plan to do that this week.

If some blocker comes up and I can't finish the PR soon, I'll close it out and reopen.

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

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

rnburn · 2018-01-25T16:26:25Z

Updated to use the configuration changes from envoyproxy/data-plane-api#386. I'm adding in testing coverage next.

rnburn · 2018-01-25T16:35:02Z

@yurishkuro @isaachier -- are you ok with the build requirements that this method of dynamic loading imposes on the tracing library? Is it a build you could see supporting for Jaeger's C++ client?

Just to summarize: What you need to do is

Link dependencies in statically (including the standard C++ library using the flag -static-libstdc++).
Set up the linker to use an export map so that only Jaeger symbols are exposed.

(I also outlined the approach in this SO post).

isaachier · 2018-01-25T16:44:14Z

I think CMake helps with the exports: https://cmake.org/cmake/help/v3.0/module/GenerateExportHeader.html. I can fix the linking issue.

mattklein123 · 2018-01-25T17:04:31Z

@rnburn re: linking, this is our first foray into dynamic linking which I think we will be doing more of in the future. We can also have a compile option for Envoy which dynamically links if that is preferable for some deployments.

rnburn · 2018-01-26T12:32:10Z

So I have a library mocktracer that implements the dynamic loading interface and is exposed to bazel.

I'm trying to figure out how to write a cc_test so that mocktracer is a dependency that's not linked in but whose location is passed to the unit test so that it can be dlopen-ed and tested against.

Any suggestions on how to do this? (also asked on SO)

rnburn · 2018-01-26T15:16:50Z

I found one way to do it: If you list a library in the data attribute for cc_test, then it will be available in the current working directory (albeit in a platform-architecture dependent location); but you can then run the find command to discover the path... not very elegant, but is there a better way?

mattklein123 · 2018-01-26T16:20:34Z

@jmillikin-stripe @htuch any idea on @rnburn question above?

htuch · 2018-01-26T16:24:40Z

See https://github.com/envoyproxy/envoy/blob/master/test/test_common/environment.h#L73; this is how you access the data files. Examples are the SSL cert tests, e.g. https://github.com/envoyproxy/envoy/blob/master/test/common/ssl/BUILD#L11 and https://github.com/envoyproxy/envoy/blob/master/test/common/ssl/ssl_socket_test.cc#L1115. In that example, the "rundir" is from a JSON template, but the first link I provided gives you access to the directory as a simple string.

rnburn · 2018-01-26T17:19:57Z

The library will show up in runfilesDirectory but the path will be something like _solib_darwin_x86_64/libexternal_Sio_Uopentracing_Ucpp_Smocktracer_Slibmocktracer.so

Is there an environmental variable for the _solib_darwin_x86_64 part of the path?

htuch · 2018-01-26T17:25:22Z

Maybe a genrule to cp the source location as given by Bazel into some sane standard location?

jmillikin-stripe · 2018-01-26T19:38:59Z

For data files with annoying complex paths like that, I've usually seen them passed to tests with the args attribute and a command-line flag. This should work with all test rules:

cc_test(
  name = "my_test",
  srcs = ["my_test.cc"],
  data = ["//important:data"],
  args = ["--some-important-data=$(location //important:data)"],
)

For C++ tests in particular, I think (based on the docs) you can also put the path into a preprocessor macro with copts=['-DIMPORTANT_DATA_PATH="$(location //important:data)"'] or defines=['IMPORTANT_DATA_PATH="$(location //important:data)"']. I've never tried this though.

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

rnburn · 2018-01-31T13:47:08Z

@jmillikin-stripe I tried using $(location ...), but it gives me a relative path that's different from where cc_test puts the library from the data parameter. It's also not accessible from the working directory of the test and runfilesDirectory.

I wanted to at least start to get some testing in, so I went forward with using an ugly find hack for now; but I'll replace it with a better solution if I can find one.

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

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

rnburn · 2018-02-26T21:15:39Z

I added a TODO for an example; I merged in opentracing/opentracing-cpp#45 and I updated the build so that it no longer points to my fork of opentracing-cpp.

I'm going to put in a few more commits to address #2252 (comment); then I think I should be caught up with everything.

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

alyssawilk · 2018-02-27T14:16:00Z

code LGTM modulo the two build failures. Clean those up and you're good to go

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

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

rnburn · 2018-02-27T22:28:21Z

Ok. I fixed the build. I also added more test coverage so that now these files from my last PR are covered:
https://28730-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_common_tracing_opentracing_driver_impl.cc.html
https://28730-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_common_tracing_lightstep_tracer_impl.cc.html

alyssawilk

LGTM. @htuch could you take a quick pass on this one before we merge?

htuch

I've left a couple of comments that can be addressed post-merge optionally.

htuch · 2018-02-28T22:00:14Z

source/common/tracing/dynamic_opentracing_driver_impl.cc

+    throw EnvoyException{formatErrorMessage(tracer_maybe.error(), error_message)};
+  }
+  tracer_ = std::move(*tracer_maybe);
+  RELEASE_ASSERT(tracer_ != nullptr);


Why is this RELEASE_ASSERT and not ASSERT?

htuch · 2018-02-28T22:00:50Z

source/common/tracing/dynamic_opentracing_driver_impl.cc

+  RELEASE_ASSERT(tracer_ != nullptr);
+}
+
+std::string DynamicOpenTracingDriver::formatErrorMessage(std::error_code error_code,


This should probably be in the anonymous namespace, it's not using any state from the instance.

htuch · 2018-02-28T22:02:08Z

source/common/tracing/dynamic_opentracing_driver_impl.cc

+
+DynamicOpenTracingDriver::DynamicOpenTracingDriver(Stats::Store& stats, const std::string& library,
+                                                   const std::string& tracer_config)
+    : OpenTracingDriver{stats} {


Tiny nit: FYI, most of Envoy doesn't use brace style when calling super.

* test envoy wasm build * circle cmake * update circle builder image * Update envoy-wasm sha to the latest * pin circle ci bazel version at 0.25.0 * update circle ci commands

* jni: fix mismatched return value types The Java side expects an `int` return value: https://github.com/envoyproxy/envoy-mobile/blob/9a397d72760acba3f3e96077f705971a8dc93d91/library/java/io/envoyproxy/envoymobile/engine/JniLibrary.java#L323 Signed-off-by: JP Simard <jp@jpsim.com>

Support dynamically loading tracing libraries.

3dfbfbc

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

Add v2 configuration support for dynamically loaded tracers.

4aa5578

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

rnburn mentioned this pull request Jan 3, 2018

tracing: Support dynamically loading tracing libraries. envoyproxy/data-plane-api#386

Merged

Don't change v1 configuration.

3473a7d

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

mattklein123 self-assigned this Jan 12, 2018

rnburn mentioned this pull request Jan 23, 2018

tracing: Support OpenTracing API #2017

Merged

rnburn added 4 commits January 25, 2018 14:27

Merge branch 'master' of https://github.com/envoyproxy/envoy into dyn…

c1ad0d3

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

s/DYNAMIC/DYNAMIC_OT/

5810042

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

Update opentracing-cpp dependency.

a8a1dad

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

Switch to use inline tracer configuration.

d0d9a12

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

Add dynamic tracer tests.

4b538a3

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

rnburn added 3 commits February 26, 2018 15:50

Merge branch 'master' of https://github.com/envoyproxy/envoy into dyn…

b6ca9ca

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

Fix format.

ace09fb

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

Add TODO note for example.

8dd4318

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

rnburn added 4 commits February 26, 2018 18:27

Add test coverage for setTag.

21c9d67

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

Add test coverage for LightStepLogger.

4567d6f

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

Move method to cc file.

1b491d9

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

Fix typo.

8f7fa81

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

alyssawilk previously approved these changes Feb 27, 2018

View reviewed changes

Fix tests for lightstep_tracer_impl.

5f2d430

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

rnburn dismissed alyssawilk’s stale review via 5f2d430 February 27, 2018 17:05

rnburn added 3 commits February 27, 2018 12:43

Fix build.

f1a158c

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

Add missing test coverage.

6878e86

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

Merge branch 'master' of https://github.com/envoyproxy/envoy into dyn…

fca28e5

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

alyssawilk approved these changes Feb 28, 2018

View reviewed changes

htuch approved these changes Feb 28, 2018

View reviewed changes

htuch merged commit da865c3 into envoyproxy:master Feb 28, 2018

alyssawilk mentioned this pull request Apr 4, 2018

repo reorg: move tracers #2983

Merged

rnburn mentioned this pull request Jul 2, 2018

Resolve #112 Build Under Windows jaegertracing/jaeger-client-cpp#115

Merged

Oberon00 mentioned this pull request Nov 5, 2018

How to build a dynamic_opentracing plugin for Envoy #4963

Closed

cce mentioned this pull request Jun 7, 2019

support of open tracing in clickhouse ClickHouse/ClickHouse#5182

Closed

dmitriyilin mentioned this pull request Nov 28, 2022

Extension mechanism for HTTP filters based on dynamic loading #24230

Open

hoolioh mentioned this pull request Feb 21, 2024

Integrating Rust binaries in tracer extensions #32502

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tracing: Support dynamically loading tracing libraries. #2252

tracing: Support dynamically loading tracing libraries. #2252

rnburn commented Dec 21, 2017 •

edited

Loading

mattklein123 commented Dec 22, 2017

mattklein123 commented Dec 22, 2017

rnburn commented Dec 22, 2017 •

edited

Loading

rnburn commented Jan 3, 2018

mattklein123 commented Jan 25, 2018

rnburn commented Jan 25, 2018

rnburn commented Jan 25, 2018

rnburn commented Jan 25, 2018

isaachier commented Jan 25, 2018

mattklein123 commented Jan 25, 2018

rnburn commented Jan 26, 2018

rnburn commented Jan 26, 2018

mattklein123 commented Jan 26, 2018

htuch commented Jan 26, 2018

rnburn commented Jan 26, 2018

htuch commented Jan 26, 2018

jmillikin-stripe commented Jan 26, 2018

rnburn commented Jan 31, 2018 •

edited

Loading

rnburn commented Feb 26, 2018

alyssawilk commented Feb 27, 2018

rnburn commented Feb 27, 2018

alyssawilk left a comment

htuch left a comment

htuch Feb 28, 2018

htuch Feb 28, 2018

htuch Feb 28, 2018

tracing: Support dynamically loading tracing libraries. #2252

tracing: Support dynamically loading tracing libraries. #2252

Conversation

rnburn commented Dec 21, 2017 • edited Loading

mattklein123 commented Dec 22, 2017

mattklein123 commented Dec 22, 2017

rnburn commented Dec 22, 2017 • edited Loading

rnburn commented Jan 3, 2018

mattklein123 commented Jan 25, 2018

rnburn commented Jan 25, 2018

rnburn commented Jan 25, 2018

rnburn commented Jan 25, 2018

isaachier commented Jan 25, 2018

mattklein123 commented Jan 25, 2018

rnburn commented Jan 26, 2018

rnburn commented Jan 26, 2018

mattklein123 commented Jan 26, 2018

htuch commented Jan 26, 2018

rnburn commented Jan 26, 2018

htuch commented Jan 26, 2018

jmillikin-stripe commented Jan 26, 2018

rnburn commented Jan 31, 2018 • edited Loading

rnburn commented Feb 26, 2018

alyssawilk commented Feb 27, 2018

rnburn commented Feb 27, 2018

alyssawilk left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

htuch Feb 28, 2018

Choose a reason for hiding this comment

htuch Feb 28, 2018

Choose a reason for hiding this comment

htuch Feb 28, 2018

Choose a reason for hiding this comment

rnburn commented Dec 21, 2017 •

edited

Loading

rnburn commented Dec 22, 2017 •

edited

Loading

rnburn commented Jan 31, 2018 •

edited

Loading