From 8ceef1217ec8487b679bf24b61ef2c65175b14e8 Mon Sep 17 00:00:00 2001 From: Trevor Schroeder Date: Mon, 15 May 2017 16:30:51 -0400 Subject: [PATCH 1/5] Hack to allow conditionally compiling out lightstep. --- bazel/BUILD | 8 ++++++++ source/common/tracing/BUILD | 12 ++++++++---- source/server/BUILD | 4 ++++ source/server/configuration_impl.cc | 7 +++++++ 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/bazel/BUILD b/bazel/BUILD index dcaf8989ebfd7..ed9b5aeab8126 100644 --- a/bazel/BUILD +++ b/bazel/BUILD @@ -31,3 +31,11 @@ config_setting( name = "disable_signal_trace", values = {"define": "signal_trace=disabled"}, ) + +# TODO(tschroed): This allows linking against Google code, but is very hacky. +# https://github.com/lyft/envoy/issues/967 to do it properly, at which point +# this can be deleted. +config_setting( + name = "disable_lightstep_tracing", + values = {"define": "lightstep_tracing=disabled"}, +) diff --git a/source/common/tracing/BUILD b/source/common/tracing/BUILD index 4fe2004c8b270..f0f93c94da554 100644 --- a/source/common/tracing/BUILD +++ b/source/common/tracing/BUILD @@ -12,12 +12,16 @@ envoy_cc_library( name = "http_tracer_lib", srcs = [ "http_tracer_impl.cc", - "lightstep_tracer_impl.cc", - ], + ] + select({ + "//third_party/envoy/src/bazel:disable_lightstep_tracing": [], + "//conditions:default": ["lightstep_tracer_impl.cc"], + }), hdrs = [ "http_tracer_impl.h", - "lightstep_tracer_impl.h", - ], + ] + select({ + "//third_party/envoy/src/bazel:disable_lightstep_tracing": [], + "//conditions:default": ["lightstep_tracer_impl.h"], + }), external_deps = ["lightstep"], deps = [ "//include/envoy/local_info:local_info_interface", diff --git a/source/server/BUILD b/source/server/BUILD index ee2d3b634349a..89210a261c502 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -20,6 +20,10 @@ envoy_cc_library( name = "configuration_lib", srcs = ["configuration_impl.cc"], hdrs = ["configuration_impl.h"], + copts = select({ + "//third_party/envoy/src/bazel:disable_lightstep_tracing": ["-DDISABLE_LIGHTSTEP_TRACING"], + "//conditions:default": [], + }), deps = [ "//include/envoy/http:filter_interface", "//include/envoy/network:connection_interface", diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 514f5e62084f2..6761ec65e4ce8 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -17,7 +17,9 @@ #include "common/ratelimit/ratelimit_impl.h" #include "common/ssl/context_config_impl.h" #include "common/tracing/http_tracer_impl.h" +#ifndef DISABLE_LIGHTSTEP_TRACING #include "common/tracing/lightstep_tracer_impl.h" ++#endif // DISABLE_LIGHTSTEP_TRACING #include "common/tracing/zipkin/zipkin_tracer_impl.h" #include "common/upstream/cluster_manager_impl.h" @@ -117,6 +119,7 @@ void MainImpl::initializeTracers(const Json::Object& configuration) { ::Runtime::RandomGenerator& rand = server_.random(); +#ifndef DISABLE_LIGHTSTEP_TRACING if (type == "lightstep") { Json::ObjectPtr lightstep_config = driver->getObject("config"); @@ -134,6 +137,10 @@ void MainImpl::initializeTracers(const Json::Object& configuration) { http_tracer_.reset( new Tracing::HttpTracerImpl(std::move(lightstep_driver), server_.localInfo())); } else { + if (type == "zipkin" ) { +#else + { +#ifndef DISABLE_LIGHTSTEP_TRACING ASSERT(type == "zipkin"); Tracing::DriverPtr zipkin_driver( From 46250a2f5b6b6b01275e2ce4e2d18ce376bb2d04 Mon Sep 17 00:00:00 2001 From: Trevor Schroeder Date: Wed, 17 May 2017 10:08:12 -0400 Subject: [PATCH 2/5] Fix up things that didn't patch correctly from teh g3 version. --- source/common/tracing/BUILD | 4 ++-- source/server/BUILD | 2 +- source/server/configuration_impl.cc | 5 ++--- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/source/common/tracing/BUILD b/source/common/tracing/BUILD index f0f93c94da554..393dab733a19a 100644 --- a/source/common/tracing/BUILD +++ b/source/common/tracing/BUILD @@ -13,13 +13,13 @@ envoy_cc_library( srcs = [ "http_tracer_impl.cc", ] + select({ - "//third_party/envoy/src/bazel:disable_lightstep_tracing": [], + "//bazel:disable_lightstep_tracing": [], "//conditions:default": ["lightstep_tracer_impl.cc"], }), hdrs = [ "http_tracer_impl.h", ] + select({ - "//third_party/envoy/src/bazel:disable_lightstep_tracing": [], + "//bazel:disable_lightstep_tracing": [], "//conditions:default": ["lightstep_tracer_impl.h"], }), external_deps = ["lightstep"], diff --git a/source/server/BUILD b/source/server/BUILD index 89210a261c502..60a5a7f5ac818 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -21,7 +21,7 @@ envoy_cc_library( srcs = ["configuration_impl.cc"], hdrs = ["configuration_impl.h"], copts = select({ - "//third_party/envoy/src/bazel:disable_lightstep_tracing": ["-DDISABLE_LIGHTSTEP_TRACING"], + "//bazel:disable_lightstep_tracing": ["-DDISABLE_LIGHTSTEP_TRACING"], "//conditions:default": [], }), deps = [ diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 6761ec65e4ce8..1449b9d8a6ebd 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -19,7 +19,7 @@ #include "common/tracing/http_tracer_impl.h" #ifndef DISABLE_LIGHTSTEP_TRACING #include "common/tracing/lightstep_tracer_impl.h" -+#endif // DISABLE_LIGHTSTEP_TRACING +#endif // DISABLE_LIGHTSTEP_TRACING #include "common/tracing/zipkin/zipkin_tracer_impl.h" #include "common/upstream/cluster_manager_impl.h" @@ -137,10 +137,9 @@ void MainImpl::initializeTracers(const Json::Object& configuration) { http_tracer_.reset( new Tracing::HttpTracerImpl(std::move(lightstep_driver), server_.localInfo())); } else { - if (type == "zipkin" ) { #else { -#ifndef DISABLE_LIGHTSTEP_TRACING +#endif // DISABLE_LIGHTSTEP_TRACING ASSERT(type == "zipkin"); Tracing::DriverPtr zipkin_driver( From b7dafdad3c0497bd1b6763970df4b5bfdeacfc61 Mon Sep 17 00:00:00 2001 From: Trevor Schroeder Date: Wed, 17 May 2017 11:36:51 -0400 Subject: [PATCH 3/5] Clang format headers. --- source/server/configuration_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc index 1449b9d8a6ebd..2736d76c52261 100644 --- a/source/server/configuration_impl.cc +++ b/source/server/configuration_impl.cc @@ -17,6 +17,7 @@ #include "common/ratelimit/ratelimit_impl.h" #include "common/ssl/context_config_impl.h" #include "common/tracing/http_tracer_impl.h" + #ifndef DISABLE_LIGHTSTEP_TRACING #include "common/tracing/lightstep_tracer_impl.h" #endif // DISABLE_LIGHTSTEP_TRACING From fb71d1f02346eeb9ce28fe48f8b252ba91bd73f2 Mon Sep 17 00:00:00 2001 From: Trevor Schroeder Date: Thu, 18 May 2017 17:06:21 -0400 Subject: [PATCH 4/5] Conditionalize the lightsep library dependency as well. In practice, it gets pruned because it's not used, but that's pretty icky. Let's not bother building at all. --- bazel/BUILD | 5 +---- source/common/tracing/BUILD | 15 +++++++++++---- source/server/BUILD | 5 ++++- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/bazel/BUILD b/bazel/BUILD index ed9b5aeab8126..f6f4348f7983d 100644 --- a/bazel/BUILD +++ b/bazel/BUILD @@ -32,10 +32,7 @@ config_setting( values = {"define": "signal_trace=disabled"}, ) -# TODO(tschroed): This allows linking against Google code, but is very hacky. -# https://github.com/lyft/envoy/issues/967 to do it properly, at which point -# this can be deleted. config_setting( - name = "disable_lightstep_tracing", + name = "disable_lightstep", values = {"define": "lightstep_tracing=disabled"}, ) diff --git a/source/common/tracing/BUILD b/source/common/tracing/BUILD index 393dab733a19a..eee2b0b603ba0 100644 --- a/source/common/tracing/BUILD +++ b/source/common/tracing/BUILD @@ -13,17 +13,19 @@ envoy_cc_library( srcs = [ "http_tracer_impl.cc", ] + select({ - "//bazel:disable_lightstep_tracing": [], + "//bazel:disable_lightstep": [], "//conditions:default": ["lightstep_tracer_impl.cc"], }), hdrs = [ "http_tracer_impl.h", ] + select({ - "//bazel:disable_lightstep_tracing": [], + "//bazel:disable_lightstep": [], "//conditions:default": ["lightstep_tracer_impl.h"], }), - external_deps = ["lightstep"], - deps = [ + deps = select({ + "//bazel:disable_lightstep": [], + "//conditions:default": [":lightstep_external_lib"], + }) + [ "//include/envoy/local_info:local_info_interface", "//include/envoy/runtime:runtime_interface", "//include/envoy/thread_local:thread_local_interface", @@ -43,3 +45,8 @@ envoy_cc_library( "//source/common/runtime:uuid_util_lib", ], ) + +envoy_cc_library( + name = "lightstep_external_lib", + external_deps = ["lightstep"], +) diff --git a/source/server/BUILD b/source/server/BUILD index 60a5a7f5ac818..3aa8724f80d8f 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -21,7 +21,10 @@ envoy_cc_library( srcs = ["configuration_impl.cc"], hdrs = ["configuration_impl.h"], copts = select({ - "//bazel:disable_lightstep_tracing": ["-DDISABLE_LIGHTSTEP_TRACING"], + # TODO(tschroed): This allows linking against Google code, but is very hacky. + # https://github.com/lyft/envoy/issues/967 to do it properly, at which point + # the -D can be removed. + "//bazel:disable_lightstep": ["-DDISABLE_LIGHTSTEP_TRACING"], "//conditions:default": [], }), deps = [ From 7ea85e7f617b016c078bc323af5f2af6c5d99d08 Mon Sep 17 00:00:00 2001 From: Trevor Schroeder Date: Thu, 18 May 2017 17:32:03 -0400 Subject: [PATCH 5/5] Factor lightstep_tracer_lib out into its own library for dependency management. --- source/common/tracing/BUILD | 24 +++++++----------------- source/server/BUILD | 6 ++++-- test/common/tracing/BUILD | 2 +- 3 files changed, 12 insertions(+), 20 deletions(-) diff --git a/source/common/tracing/BUILD b/source/common/tracing/BUILD index eee2b0b603ba0..2646d2d323120 100644 --- a/source/common/tracing/BUILD +++ b/source/common/tracing/BUILD @@ -10,22 +10,9 @@ envoy_package() envoy_cc_library( name = "http_tracer_lib", - srcs = [ - "http_tracer_impl.cc", - ] + select({ - "//bazel:disable_lightstep": [], - "//conditions:default": ["lightstep_tracer_impl.cc"], - }), - hdrs = [ - "http_tracer_impl.h", - ] + select({ - "//bazel:disable_lightstep": [], - "//conditions:default": ["lightstep_tracer_impl.h"], - }), - deps = select({ - "//bazel:disable_lightstep": [], - "//conditions:default": [":lightstep_external_lib"], - }) + [ + srcs = ["http_tracer_impl.cc"], + hdrs = ["http_tracer_impl.h"], + deps = [ "//include/envoy/local_info:local_info_interface", "//include/envoy/runtime:runtime_interface", "//include/envoy/thread_local:thread_local_interface", @@ -47,6 +34,9 @@ envoy_cc_library( ) envoy_cc_library( - name = "lightstep_external_lib", + name = "lightstep_tracer_lib", + srcs = ["lightstep_tracer_impl.cc"], + hdrs = ["lightstep_tracer_impl.h"], external_deps = ["lightstep"], + deps = [":http_tracer_lib"], ) diff --git a/source/server/BUILD b/source/server/BUILD index 3aa8724f80d8f..600fc005d2eab 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -27,7 +27,10 @@ envoy_cc_library( "//bazel:disable_lightstep": ["-DDISABLE_LIGHTSTEP_TRACING"], "//conditions:default": [], }), - deps = [ + deps = select({ + "//bazel:disable_lightstep": [], + "//conditions:default": ["//source/common/tracing:lightstep_tracer_lib"], + }) + [ "//include/envoy/http:filter_interface", "//include/envoy/network:connection_interface", "//include/envoy/network:filter_interface", @@ -43,7 +46,6 @@ envoy_cc_library( "//source/common/network:utility_lib", "//source/common/ratelimit:ratelimit_lib", "//source/common/ssl:context_config_lib", - "//source/common/tracing:http_tracer_lib", "//source/common/tracing/zipkin:zipkin_lib", "//source/common/upstream:cluster_manager_lib", ], diff --git a/test/common/tracing/BUILD b/test/common/tracing/BUILD index 1f525bd0e2dde..e4bd95b70ae36 100644 --- a/test/common/tracing/BUILD +++ b/test/common/tracing/BUILD @@ -21,7 +21,7 @@ envoy_cc_test( "//source/common/http:message_lib", "//source/common/runtime:runtime_lib", "//source/common/runtime:uuid_util_lib", - "//source/common/tracing:http_tracer_lib", + "//source/common/tracing:lightstep_tracer_lib", "//test/mocks/http:http_mocks", "//test/mocks/local_info:local_info_mocks", "//test/mocks/runtime:runtime_mocks",