tracing: Support OpenTracing API #2017

rnburn · 2017-11-08T01:37:03Z

Description:

Adds an OpenTracingDriver that implements Tracing::Driver using the OpenTracing API. With it, an OpenTracing-compliant tracer can be added into envoy by deriving from OpenTracingDriver and implementing the abstract tracer method, reducing the amount of code that must be maintained for each tracer implementation.
Updates the LightStep tracer to use OpenTracingDriver. The latest version of LightStep provides a customizable transport, so the code that implements an Envoy-specific Reporter and interacts directly with LightStep's protobuf classes was removed in order to bind less tightly to the LightStep code.
Adds a new tracing log category and logs warning when injection/extraction fails, or the LightStep tracer generates an error message.

A few notes:

As a step towards standardizing on a single OpenTracing header for propagation, we added optional support for single-header propagation to the LightStep tracer and switched Envoy to use that instead of its custom propagation code. Currently, LightStep's single-header propagation uses the same header as Envoy; though the expectation is that it will change to match whatever standard format emerges in the future.
Envoy still supports custom single-header propagation (though in a more OpenTracing-friendly manner). Individual tracers can be configured to use either Envoy's single-header propagation format, the tracer's native propagation format, or both via the tracer's driver class.
To support more efficient span context propagation when a tracer's headers belong to Envoy's special O(1) list, I added the function Http::HeaderMap::lookup that takes a header key and uses the StaticLookupTable to find the header's entry. If the tracer's headers don't belong to the O(1) list, propagation will still work; but the tracer will have to iterate through all of the entries.
The bazel build sets CC to point to a C++ compiler. This causes cmake to fail since it expects CC to be a C compiler. Even though C compilers aren't used, I added CC="" to opentracing.genrule_cmd and lightstep.genrule_cmd in order to work around the problem. (Not sure if there's a better solution.)

Fixes #1196

Risk Level: High

Testing:

Built and ran automated tests both locally on OSX and through the docker ci targets.
Set up a small tracing example and confirmed that traces are reported to LightStep.
Added additional unit tests for the new Http::HeaderMap::lookup function.

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

mattklein123 · 2017-11-09T00:47:52Z

Thanks @rnburn. This is great. I will spend time with this tomorrow morning.

mattklein123 · 2017-11-09T17:42:02Z

The bazel build sets CC to point to a C++ compiler. This causes cmake to fail since it expects CC to be a C compiler. Even though C compilers aren't used, I added CC="" to opentracing.genrule_cmd and lightstep.genrule_cmd in order to work around the problem. (Not sure if there's a better solution.)

@jmillikin-stripe this is related I think to our other conversation yesterday about the warnings. If possible can you review the bazel portion of this change? @htuch also? I will review the code.

jmillikin-stripe · 2017-11-09T18:07:23Z

That opentracing/lightstep build stuff looks painful. Lets see if we can convince them to put some BUILD files in their repo. Their libraries don't look complicated, so hopefully they'll be OK with it.

I'll send them PRs and link them here.

jmillikin-stripe · 2017-11-09T18:45:34Z

OpenTracing PR: opentracing/opentracing-cpp#37

rnburn · 2017-11-09T19:11:04Z

Thanks. I'm fine with supporting bazel builds in those projects. I just merged in the PR for opentracing.

jmillikin-stripe · 2017-11-09T21:01:31Z

@rnburn Thanks, that's great to know! I've sent another PR for Lightstep's C++ tracer: lightstep/lightstep-tracer-cpp#85

The BUILD files for Lightstep are a bit strange because it vendors part of googleapis, and it assumes that the Lightstep proto headers are in the system include path. It'd be nice to clean that weirdness up at some point.

rnburn · 2017-11-09T21:10:04Z

Thanks.

The googleapis were copied in because they're not installed anywhere as part of gRPC's build for C++.

And putting the lightstep proto headers in the system include was done so that the compiler warnings won't apply to them (which they will if they're included normally).

jmillikin-stripe · 2017-11-09T21:12:33Z

Sure, but #include <lightstep-tracer-common/collector.pb.h> would keep the same warning-silencing behavior without quite as much :stonk:.

mattklein123

This is cool stuff. Some comments to get started. I'm going to need to take multiple passes through this as we start making changes. Thank you for your effort on this.

mattklein123 · 2017-11-09T20:36:01Z

source/common/tracing/lightstep_tracer_impl.cc

-  span_.SetOperationName(operation);
+    Grpc::Common::chargeStat(*driver_.cluster(), lightstep::CollectorServiceFullName(),
+                             lightstep::CollectorMethodName(), true);
+    if (!Grpc::Common::deserializeBody(response->bodyAsString(), *active_response_)) {


This involves multiple copies of the body through the entire code path. You should be able to use https://github.com/envoyproxy/envoy/blob/master/source/common/buffer/zero_copy_input_stream_impl.h after draining off the first 5 bytes of the response. Take a look at how this is used elsewhere in similar situations. Then you can remove the added deserializeBody function.

mattklein123 · 2017-11-09T20:50:30Z

source/common/tracing/lightstep_tracer_impl.cc

+                                   lightstep::CollectorMethodName());
+  message->body() = Grpc::Common::serializeBody(request);
+
+  uint64_t timeout =


mattklein123 · 2017-11-09T20:51:59Z

source/common/tracing/lightstep_tracer_impl.cc


-void LightStepRecorder::enableTimer() {
+void LightStepDriver::TlsLightStepTracer::enableTimer() {
  uint64_t flush_interval =


mattklein123 · 2017-11-09T20:52:46Z

source/common/tracing/lightstep_tracer_impl.cc

+    tls_options.use_single_key_propagation = true;
+    tls_options.logger_sink = LightStepLogger{};
+    tls_options.max_buffered_spans = std::function<size_t()>{[this] {
+      auto result = runtime_.snapshot().getInteger("tracing.lightstep.min_flush_spans", 5U);


nit: just return, don't need local var

mattklein123 · 2017-11-09T20:58:27Z

source/common/http/header_map_impl.cc

@@ -399,6 +399,23 @@ void HeaderMapImpl::iterateReverse(ConstIterateCb cb, void* context) const {
  }
 }

+HeaderMap::Lookup HeaderMapImpl::lookup(const LowerCaseString& key,
+                                        const HeaderEntry** entry) const {
+  StaticLookupEntry::EntryCb cb = ConstSingleton<StaticLookupTable>::get().find(key.get().c_str());


Could we potentially have a findConst() or something which returns a ConstEntryCb so that we don't need the const_cast below?

Via a macro, each of the O(1) headers adds a lambda funtion that takes HTTPHeaderMapImpl& as an argument. The only way I see around const_cast is to have them also add entries for a const HTTPHeaderMapImpl& lambda.

IMO const_cast is the better option.

I could go either way. I think it's slightly cleaner to have the init code add a const callback and then have a fineConst(), but I'm ok with this. I would just add a comment on the tradeoff.

Added comment for const_cast.

nit: For the next person that comes along and asks "why const_cast" can you add a brief additional comment on the tradeoffs between this and ^.

Added explanation for choosing const_cast.

mattklein123 · 2017-11-09T21:07:30Z

source/common/tracing/opentracing_driver_impl.cc

+SpanPtr OpenTracingSpan::spawnChild(const Config&, const std::string& name, SystemTime start_time) {
+  std::unique_ptr<opentracing::Span> ot_span = span_->tracer().StartSpan(
+      name, {opentracing::ChildOf(&span_->context()), opentracing::StartTimestamp(start_time)});
+  if (ot_span == nullptr) {


when does this return nullptr?

Most of the OpenTracing API is noexcept. LightStep's StartSpan would only return nullptr if something like std::bad_alloc was thrown while it was trying to allocate memory. I could change this to use RELEASE_ASSERT since nullptr would likely mean the application is on the verge of crashing.

Yeah preference to either use RELEASE_ASSERT for these types of things or in the case of nullptr just let Envoy crash via null-ptr dereference.

mattklein123 · 2017-11-09T21:10:55Z

source/common/tracing/opentracing_driver_impl.cc

+    const opentracing::expected<void> was_successful =
+        span_->tracer().Inject(span_->context(), oss);
+    if (!was_successful) {
+      ENVOY_LOG(warn, "Failed to inject span context: {}", was_successful.error().message());


Same comments as below on error handling (sorry comments are coming out of order). In general, error handling should:

not log above debug if it can happen via user supplied input

stat

if it's something that just shouldn't happen, it should be an ASSERT or possibly RELEAS_ASSERT (please see error handling section in style guide)

Changed to log at debug level, but still need to add a stat.

mattklein123 · 2017-11-09T21:11:03Z

source/common/tracing/opentracing_driver_impl.cc

+void OpenTracingSpan::injectContext(Http::HeaderMap& request_headers) {
+  if (use_single_header_propagation_) {
+    // Inject the span context using Envoy's single-header format.
+    std::ostringstream oss;


same comment as below about string stream

mattklein123 · 2017-11-09T21:12:29Z

source/common/tracing/opentracing_driver_impl.cc

+                parent_span_ctx_maybe.error().message());
+    }
+  } else if (use_tracer_propagation) {
+    const OpenTracingHTTPHeadersReader reader{request_headers};


From looking through, I think LS driver uses this path, but not the other one. Do we have Envoy test coverage to 100% on the other paths? (Please see coverage report in CI). We will need coverage for both paths.

Also, what header is the LS driver actually using in the current version when it goes through this path? x-ot-span-ctx? Or some new header? It's not super clear.

I'll add test coverage for the other path.

It's using the same x-ot-span-context header.

Added test coverage for the other code path.

mattklein123 · 2017-11-09T21:14:25Z

source/common/tracing/opentracing_driver_impl.h

+namespace Envoy {
+namespace Tracing {
+
+class OpenTracingSpan : public Span, Logger::Loggable<Logger::Id::tracing> {


Can we have some links to docs here or just a set of comments somewhere on how a driver is actually created? It's a little confusing to me re: what the various propagation modes are, what the driver implements, how the header lookup code works (in terms of what the OT code expects), etc.

Added documentation.

rnburn · 2017-11-09T21:45:22Z

@jmillikin-stripe -- yeah, the path could be changed like that. I just meant having them included with -isystem instead of -I.

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

jmillikin-stripe · 2017-11-10T00:47:16Z

I just verified that the following Bazel repository entries work as expected with the new Lightstep/OpenTracing build rules:

  native.git_repository(
      name = "io_opentracing_cpp",
      remote = "https://github.com/opentracing/opentracing-cpp",
      commit = "713c15d40ae63185d2bec99bf3b03823967d7108",
  )
  native.git_repository(
      name = "com_lightstep_tracer_cpp",
      remote = "https://github.com/lightstep/lightstep-tracer-cpp",
      commit = "LIGHTSTEP_COMMIT_HERE",
  )
  native.new_git_repository(
      name = "com_lightstep_tracer_common",
      remote = "https://github.com/lightstep/lightstep-tracer-common",
      commit = "3943f3f04e3547e7e68d7900ef2efde045fa8901",
      build_file = "@com_lightstep_tracer_cpp//:lightstep-tracer-common/BUILD",
  )
  native.new_git_repository(
      name = "lightstep_vendored_googleapis",
      remote = "https://github.com/google/googleapis",
      commit = "d6f78d948c53f3b400bb46996eb3084359914f9b",
      build_file = "@com_lightstep_tracer_cpp//:lightstep-tracer-common/third_party/googleapis/BUILD",
  )
  native.bind(
      name="lightstep",
      actual="@com_lightstep_tracer_cpp//:lightstep_tracer",
  )

For the things that lightstep-tracer-cpp is vendoring, I used the commits in the LS readme files.

htuch

Neat stuff.

htuch · 2017-11-13T03:38:59Z

include/envoy/http/header_map.h

+  enum class Lookup { Found, NotFound, NotSupported };
+
+  /**
+   * Lookup one of the O(1) headers by key.


It would be clearer to refer to these as something like "predefined inline header (see ALL_INLINE_HEADERS below)". Using the complexity alone elides the semantic info here.

Changed naming.

htuch · 2017-11-13T03:45:16Z

source/common/tracing/lightstep_tracer_impl.cc

-void LightStepSpan::finishSpan() { span_.Finish(); }
+void LightStepDriver::LightStepTransporter::onSuccess(Http::MessagePtr&& response) {
+  try {
+    Grpc::Common::validateResponse(*response);


Envoy has a gRPC client layered above Http::AsyncClient in https://github.com/envoyproxy/envoy/blob/master/source/common/grpc/async_client_impl.h. Can you make use of this? This would avoid having to hand parse out gRPC messages..

Grpc::AsyncClientImpl can't be used with an abstract base class for the response since it constructs an instance of the class.

I'd rather not expose LightStep's protobuf message types directly since that would provide less encapsulation and potentially break code using the library if they were to change.

I discussed this a while ago with @htuch and although I think we could fixup the gRPC async client to work OK with raw messages, don't worry about this for now. I would potentially just add a TODO to look into cleaning this up in the future.

Added TODO note.

htuch · 2017-11-13T03:49:01Z

source/common/tracing/opentracing_driver_impl.cc

+    }
+  }
+
+  opentracing::expected<void> ForeachKey(OpenTracingCb f) const override {


Nit: forEachKey.

This function comes from the the OT API and since it's override a virtual function the name has to match.

htuch · 2017-11-13T03:49:29Z

source/common/tracing/opentracing_driver_impl.cc

+private:
+  const Http::HeaderMap& request_headers_;
+
+  static Http::HeaderMap::Iterate header_map_callback(const Http::HeaderEntry& header,


Nit: headerMapCallback.

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

jmillikin-stripe · 2017-11-13T20:53:21Z

bazel/external/BUILD

@@ -4,7 +4,6 @@ cc_library(
    name = "all_external",
    srcs = [":empty.cc"],
    deps = [
-        "@com_github_lightstep_lightstep_tracer_cpp//:lightstep",


This target is used to pre-build the dependencies when generating the Docker image. We still want to pre-build Lightstep and all its deps since they're pretty big.

jmillikin-stripe · 2017-11-13T20:56:12Z

bazel/repositories.bzl

@@ -312,23 +313,36 @@ def _com_github_gcovr_gcovr():
        actual = "@com_github_gcovr_gcovr//:gcovr",
    )

+def _com_github_opentracing_opentracing_cpp():
+    location = REPOSITORY_LOCATIONS[


Please use _repository_impl() here so that downstream builders of Envoy can override the dependency definition.

_repository_impl("io_opentracing_cpp") native.bind( name = "opentracing", actual = "@io_opentracing_cpp//:opentracing", )

You'll want to adjust the repository_locations.bzl name to match.

Changed to use _repository_impl and matched names to keys in REPOSITORY_LOCATIONS.

jmillikin-stripe · 2017-11-13T20:58:26Z

bazel/repositories.bzl

-        ],
-        genrule_cmd_file = "@envoy//bazel/external:lightstep.genrule_cmd",
-        build_file = "@envoy//bazel/external:lightstep.BUILD",
+    native.git_repository(


jmillikin-stripe · 2017-11-13T21:00:07Z

bazel/repository_locations.bzl

-        urls = ["https://github.com/lightstep/lightstep-tracer-cpp/releases/download/v0_36/lightstep-tracer-cpp-0.36.tar.gz"],
+        commit = "c9d9215b5c3652c7eb5640903697d9a683e1df76",
+        remote = "https://github.com/lightstep/lightstep-tracer-cpp",
+        googleapis = dict(


It's pretty confusing to have this extra level of nesting. How about giving googleapis its own location?

Separated out. Is it ok to build target in the same function as lightstep?

I think so. The functions only exist for human readability, and it makes sense to have all the lightstep stuff in one place.

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

rnburn · 2017-11-28T01:08:32Z

@mattklein123 -- this should be ready for another round of reviewing.

I think I addressed all the comments except for the concerns on stringstream performance.

There were some objections on opentracing/opentracing-cpp#42 to adding more direct binary propagation support for strings. I changed the span context extraction code to use a more efficient version of std::istream that doesn't copy the data string (unlike std::istringstream), but it's still using std::ostringstream for injection since I don't know that there's an easy way to make a faster replacement (see StackOverflow).

mattklein123 · 2017-11-28T21:29:42Z

@rnburn FYI I'm trying to get the 1.5.0 release out the door so I might be a bit delayed in looking at this. Sorry. It's on my backlog.

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

mattklein123

This is looking solid. A bunch of random nits. Excited to move this forward.

@lita: Can you please work with @goaway and @junr03 to smoke test this change on a front Envoy edge node at Lyft. This is a pretty scary change and I would like to get a quick sanity test on this before we merge. Thank you!

mattklein123 · 2017-12-05T22:17:29Z

source/common/common/utility.h

+/**
+ * Class used for creating non-copying std::istream's. See IMemoryStream below.
+ */
+class MemoryStreamBuffer : public std::streambuf {


nit: Can we call this ConstMemoryStreamBuffer ?

mattklein123 · 2017-12-05T22:18:00Z

source/common/common/utility.h

+ *
+ * See https://stackoverflow.com/a/13059195/4447365.
+ */
+class IMemoryStream : public virtual MemoryStreamBuffer, public std::istream {


nit: InputConstMemoryStream ?

mattklein123 · 2017-12-05T22:18:55Z

source/common/http/header_map_impl.cc

@@ -399,6 +399,23 @@ void HeaderMapImpl::iterateReverse(ConstIterateCb cb, void* context) const {
  }
 }

+HeaderMap::Lookup HeaderMapImpl::lookup(const LowerCaseString& key,
+                                        const HeaderEntry** entry) const {
+  StaticLookupEntry::EntryCb cb = ConstSingleton<StaticLookupTable>::get().find(key.get().c_str());


nit: For the next person that comes along and asks "why const_cast" can you add a brief additional comment on the tradeoffs between this and ^.

mattklein123 · 2017-12-05T22:19:50Z

source/common/tracing/lightstep_tracer_impl.cc

+namespace {
+class LightStepLogger : Logger::Loggable<Logger::Id::tracing> {
+public:
+  void operator()(lightstep::LogLevel level, opentracing::string_view message) const {


I'm not familiar with opentracing::string_view. Can this by passed by const reference? Same Q in a bunch of other places.

It could, but it's usually faster to take small objects like string_view by value.

Also, passing by value will be the convention for std::string_view and is what's done in the C++17 standard library. (See for example operator<<)

I'm not really sure how it can be faster to pass small objects by value vs. reference (would be curious if you have doc/article I can read on that), but totally fine to leave as is if that's what C++17 will do.

The members of small objects can get split and passed in registers, so accessing std::string_view::data can be a simple read from the register. If it's passed by const std::string_view& then it needs to go through an extra dereference to access std::string_view::data. You can see it in the generated assembly.

template <typename T> void do_not_optimize_away(T const& val) { asm volatile("" : : "g"(val) : "memory"); } void f1(std::string_view s) { char x = *s.data(); char y = x + (char)1; do_not_optimize_away(y); } void f2(const std::string_view& s) { char x = *s.data(); char y = x + (char)1; do_not_optimize_away(y); }

compiled with g++ -O3 -S shows

f1:

.globl __Z2f1St17basic_string_viewIcSt11char_traitsIcEE __Z2f1St17basic_string_viewIcSt11char_traitsIcEE: LFB758: movzbl (%rsi), %eax addl $1, %eax ret

and f2:

.globl __Z2f2RKSt17basic_string_viewIcSt11char_traitsIcEE __Z2f2RKSt17basic_string_viewIcSt11char_traitsIcEE: LFB759: movq 8(%rdi), %rax movzbl (%rax), %eax addl $1, %eax ret

Note the extra dereferencing instructions in f2.

Got it, that makes sense. Thanks for the explanation!

mattklein123 · 2017-12-05T22:22:41Z

source/common/tracing/lightstep_tracer_impl.cc

-                                     Event::Dispatcher& dispatcher)
-    : builder_(tracer), driver_(driver) {
+LightStepDriver::TlsLightStepTracer::TlsLightStepTracer(
+    std::shared_ptr<lightstep::LightStepTracer>&& tracer, LightStepDriver& driver,


nit: moving shared_ptr is a little bit of an anti-pattern IMO. I would let compiler optimize this and just pass normally.

mattklein123 · 2017-12-05T22:25:28Z

source/common/tracing/lightstep_tracer_impl.h


  Upstream::ClusterManager& clusterManager() { return cm_; }
  Upstream::ClusterInfoConstSharedPtr cluster() { return cluster_; }
  Runtime::Loader& runtime() { return runtime_; }
  LightstepTracerStats& tracerStats() { return tracer_stats_; }
+  void setPropagationMode(PropagationMode propagation_mode) {


Is there any reason this can't be passed via constructor?

I can add it as a constructor argument also. I only put it there for testing to get coverage for the different versions of Inject/Extract.

In the tests can you use constructor to construct a new driver? IMO it's cleaner to have this be part of object creation and not settable. If it's really required for testing please call it setPropagationModeForTest

Changed to use constructor.

mattklein123 · 2017-12-05T22:26:02Z

source/common/tracing/lightstep_tracer_impl.h

-  // Tracer::TracingDriver
-  SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers,
-                    const std::string& operation_name, SystemTime start_time) override;
+  // Tracer::OpenTracingDriver


super nit: overrides (with comment) go below other functions in a specific visibility scope (so it's clear what is what).

Rearranged.

mattklein123 · 2017-12-05T22:27:24Z

source/common/tracing/opentracing_driver_impl.cc

+      return opentracing::make_unexpected(opentracing::key_not_found_error);
+    case Http::HeaderMap::Lookup::NotSupported:
+      return opentracing::make_unexpected(opentracing::lookup_key_not_supported_error);
+    default:


just remove the default, and put the NOT_REACHED at the end of the function.

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

mattklein123

LGTM. Thank you @rnburn! We will get this some tested early this week.

mattklein123 · 2017-12-11T03:40:54Z

source/common/tracing/lightstep_tracer_impl.h

@@ -105,7 +103,7 @@ class LightStepDriver : public OpenTracingDriver {
  ThreadLocal::SlotPtr tls_;
  Runtime::Loader& runtime_;
  std::unique_ptr<lightstep::LightStepTracerOptions> options_;
-  PropagationMode propagation_mode_ = PropagationMode::TracerNative;
+  PropagationMode propagation_mode_;


This can be const now I think.

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

mattklein123

@rnburn @lita just finished up a smoke test. Things look good. I took a look a perf profile and this does already look better than the old setup from an allocation/copy perspective, but I think there is more we can do here. We can chat about it offline and iterate.

Thank you for seeing this through!!!

rnburn · 2017-12-16T00:45:41Z

@mattklein123 Thanks. Improving the performance is definitely something I'm going to look into. I expect there are quite a few cases in the OpenTracing API where we can use move semantics to avoid unnecessary allocations.

moderation · 2017-12-18T18:02:01Z

@rnburn what should trigger a change in the commit versions for opentracing_cpp and lightstep_tracer_cpp? Should this match to releases for the project or should there be some other trigger to update? They are currently:

    io_opentracing_cpp = dict(
        commit = "550c686e0e174c845a034f432a6c31a808f5f994",
        remote = "https://github.com/opentracing/opentracing-cpp",
    ),
    com_github_lightstep_lightstep_tracer_cpp = dict(
        commit = "deb5284395075028c3e5b4eab1416fe1e597bdb7",
        remote = "https://github.com/lightstep/lightstep-tracer-cpp",
),

lightstep_tracer_cpp is 12 commits ahead of the latest release and opentracing_cpp is behind the latest minor version bump but ahead of the latest release.

rnburn · 2017-12-18T20:21:10Z

I put in a release for the lightstep tracer for the bazel support so that envoy is now synced to 0.6.0.

I also put in #2226 to update opentracing to match the commit for version 1.2.0.

abbi-gaurav · 2018-01-23T14:17:06Z

Does it also provides the capability to use jaeger as a tracing solution since it is based on OpenTracing standard?

rnburn · 2018-01-23T15:17:12Z

@abbi-gaurav Not yet. Jaeger has some compatibility with Zipkin so you can report spans to it using the Zipkin tracer (example), but it won't interoperate with other jaeger tracing without changes to the setup and it won't support sampling.

I'm working on a PR (#2252) that will let you dynamically load in a tracer library, which would allow you to use Jaeger's C++ client directly. I have a small example that shows how it can work, but it's still under development.

* Add a new TCP cluster rewrite filter This commit adds a new TCP cluster rewrite filter which allows users to rewrite TCP cluster names obtained via TLS SNI by matching via regex configuration. Signed-off-by: Venil Noronha <veniln@vmware.com> * Make TCP cluster rewrite stackable on SNI filter This commit updates the TCP Cluster Rewrite filter to be stackable on the SNI Cluster filter. Signed-off-by: Venil Noronha <veniln@vmware.com>

This commit adds a new TCP cluster rewrite filter which allows users to rewrite TCP cluster names obtained via TLS SNI by matching via regex configuration. Signed-off-by: Venil Noronha <veniln@vmware.com>

* Improve performance by removing MD5 for check cache keys (envoyproxy#2002) * Improve performance by removing MD5 for check cache keys Signed-off-by: Wayne Zhang <qiwzhang@google.com> * not to allocate memory from stack Signed-off-by: Wayne Zhang <qiwzhang@google.com> * Make debug string readable Signed-off-by: Wayne Zhang <qiwzhang@google.com> * alts: remove ALTS (envoyproxy#2003) Signed-off-by: Lizan Zhou <lizan@tetrate.io> * Use std::hash for check cache. (envoyproxy#2009) Signed-off-by: Wayne Zhang <qiwzhang@google.com> * Remove tests to compare signature values (envoyproxy#2015) Signed-off-by: Wayne Zhang <qiwzhang@google.com> * update sample envoy config to latest version (envoyproxy#2016) * Add a new TCP cluster rewrite filter (envoyproxy#2017) * Add a new TCP cluster rewrite filter This commit adds a new TCP cluster rewrite filter which allows users to rewrite TCP cluster names obtained via TLS SNI by matching via regex configuration. Signed-off-by: Venil Noronha <veniln@vmware.com> * Make TCP cluster rewrite stackable on SNI filter This commit updates the TCP Cluster Rewrite filter to be stackable on the SNI Cluster filter. Signed-off-by: Venil Noronha <veniln@vmware.com> * Update TCP Cluster Rewrite filter name (envoyproxy#2019) This commit updates the TCP Cluster Rewrite filter name to envoy.filters.network.tcp_cluster_rewrite. Signed-off-by: Venil Noronha <veniln@vmware.com> * Enable TCP Cluster Rewrite filter registration (envoyproxy#2021) This commit enables the static registration of the TCP Cluster Rewrite filter by updating the build configuration. Signed-off-by: Venil Noronha <veniln@vmware.com> * Update Envoy SHA to 4ef8562 (envoyproxy#2023) Envoy /server_info API was inconsistent intermittently causing errors on a Proxy update on Istio. This update will bring in the API fix to Istio. Signed-off-by: Venil Noronha <veniln@vmware.com> * add proxy postsubmit periodic (envoyproxy#2025)

* Improve performance by removing MD5 for check cache keys (envoyproxy#2002) * Improve performance by removing MD5 for check cache keys Signed-off-by: Wayne Zhang <qiwzhang@google.com> * not to allocate memory from stack Signed-off-by: Wayne Zhang <qiwzhang@google.com> * Make debug string readable Signed-off-by: Wayne Zhang <qiwzhang@google.com> * alts: remove ALTS (envoyproxy#2003) Signed-off-by: Lizan Zhou <lizan@tetrate.io> * Use std::hash for check cache. (envoyproxy#2009) Signed-off-by: Wayne Zhang <qiwzhang@google.com> * Remove tests to compare signature values (envoyproxy#2015) Signed-off-by: Wayne Zhang <qiwzhang@google.com> * update sample envoy config to latest version (envoyproxy#2016) * Add a new TCP cluster rewrite filter (envoyproxy#2017) * Add a new TCP cluster rewrite filter This commit adds a new TCP cluster rewrite filter which allows users to rewrite TCP cluster names obtained via TLS SNI by matching via regex configuration. Signed-off-by: Venil Noronha <veniln@vmware.com> * Make TCP cluster rewrite stackable on SNI filter This commit updates the TCP Cluster Rewrite filter to be stackable on the SNI Cluster filter. Signed-off-by: Venil Noronha <veniln@vmware.com> * Update TCP Cluster Rewrite filter name (envoyproxy#2019) This commit updates the TCP Cluster Rewrite filter name to envoy.filters.network.tcp_cluster_rewrite. Signed-off-by: Venil Noronha <veniln@vmware.com> * Enable TCP Cluster Rewrite filter registration (envoyproxy#2021) This commit enables the static registration of the TCP Cluster Rewrite filter by updating the build configuration. Signed-off-by: Venil Noronha <veniln@vmware.com> * Update Envoy SHA to 4ef8562 (envoyproxy#2023) Envoy /server_info API was inconsistent intermittently causing errors on a Proxy update on Istio. This update will bring in the API fix to Istio. Signed-off-by: Venil Noronha <veniln@vmware.com> * add proxy postsubmit periodic (envoyproxy#2025) * Update Envoy SHA to c41fa71 (envoyproxy#2029) * Update Envoy SHA Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com> * Fix format. Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com> * bazel: Allow to distdir all dependencies (envoyproxy#2034) To use --distdir option of Bazel (which allows to use previously fetched tarballs instead of downloading dependencies during build), all dependencies should use http instead of git and need to have sha256 sums specified. Signed-off-by: Michal Rostecki <mrostecki@suse.de> * bazel: Remove BoringSSL repository (envoyproxy#2035) Pull request envoyproxy#2002 removed signature calculation which was using BoringSSL as a dependency. BoringSSL is not needed anymore. Signed-off-by: Michal Rostecki <mrostecki@suse.de> * Update Envoy SHA to fcc68f1 (envoyproxy#2037) * Update Envoy SHA to fcc68f1 Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com> * Update SHA256 Signed-off-by: JimmyCYJ <jimmychen.0102@gmail.com>

rnburn mentioned this pull request Nov 8, 2017

Support new OpenTracing API #1196

Closed

rnburn force-pushed the master branch 3 times, most recently from 8d67e8f to 3f2ed0d Compare November 8, 2017 19:42

Support OpenTracing API.

773211e

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

rnburn force-pushed the master branch from bed1cd1 to 773211e Compare November 8, 2017 21:08

Resolve merge conflicts.

e57a0fd

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

mattklein123 reviewed Nov 9, 2017

View reviewed changes

rnburn added 5 commits November 9, 2017 14:57

Const correctness fixes.

4af99ef

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

Use PURE instead of "= 0".

93ab508

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

Log Inject/Extract failures at debug level.

ad4062a

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

Avoid copying when deserializing LightStep response.

739cd54

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

Change propagation options to be const.

6270b3f

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

htuch reviewed Nov 13, 2017

View reviewed changes

Merge with build modifications.

4e03f60

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

jmillikin-stripe reviewed Nov 13, 2017

View reviewed changes

Fix lightstep build.

ab3db85

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

rnburn added 3 commits November 22, 2017 14:55

Document OpenTracingDriver.

961e195

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

Control span context propagation with enum instead of bools.

ecf6df1

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

Avoid copy associated with std::istringstream.

0803ff2

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

Ensure span-context headers aren't duplicated.

5d62c51

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

mattklein123 added this to the 1.6.0 milestone Dec 4, 2017

mattklein123 reviewed Dec 5, 2017

View reviewed changes

rnburn added 7 commits December 6, 2017 11:29

Remove default case from switch statment.

3ff6aaf

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

s/MemoryStreamBuffer/ConstMemoryStreamBuffer/

cc6a7e2

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

s/IMemoryStream/InputConstMemoryStream/

739bf4a

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

Fix ordering of member functions.

cf7896b

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

Document reason for choosing const_cast.

ce00ac6

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

Don't move from std::shared_ptr.

861e87c

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

Set propagation mode via constructor.

562746e

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

mattklein123 reviewed Dec 11, 2017

View reviewed changes

Make propagation_mode_ const.

a9411ad

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

mattklein123 mentioned this pull request Dec 14, 2017

Add Trace Service Support. envoyproxy/data-plane-api#345

Merged

mattklein123 approved these changes Dec 16, 2017

View reviewed changes

mattklein123 merged commit d586507 into envoyproxy:master Dec 16, 2017

cce mentioned this pull request Jun 7, 2019

support of open tracing in clickhouse ClickHouse/ClickHouse#5182

Closed

tracing: Support OpenTracing API #2017

tracing: Support OpenTracing API #2017

Conversation

rnburn commented Nov 8, 2017

mattklein123 commented Nov 9, 2017

mattklein123 commented Nov 9, 2017

jmillikin-stripe commented Nov 9, 2017

jmillikin-stripe commented Nov 9, 2017

rnburn commented Nov 9, 2017

jmillikin-stripe commented Nov 9, 2017

rnburn commented Nov 9, 2017

jmillikin-stripe commented Nov 9, 2017

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

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

rnburn Nov 22, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnburn commented Nov 9, 2017 • edited Loading

jmillikin-stripe commented Nov 10, 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

rnburn Nov 14, 2017 • edited Loading

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

rnburn commented Nov 28, 2017 • edited Loading

mattklein123 commented Nov 28, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnburn Dec 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnburn Nov 22, 2017 •

edited

Loading

rnburn commented Nov 9, 2017 •

edited

Loading

rnburn Nov 14, 2017 •

edited

Loading

rnburn commented Nov 28, 2017 •

edited

Loading

rnburn Dec 5, 2017 •

edited

Loading