Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tracing: Support dynamically loading tracing libraries. #2252

Merged
merged 38 commits into from
Feb 28, 2018
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
3dfbfbc
Support dynamically loading tracing libraries.
rnburn Dec 21, 2017
4aa5578
Add v2 configuration support for dynamically loaded tracers.
rnburn Jan 3, 2018
3473a7d
Don't change v1 configuration.
rnburn Jan 9, 2018
c1ad0d3
Merge branch 'master' of https://github.com/envoyproxy/envoy into dyn…
rnburn Jan 25, 2018
5810042
s/DYNAMIC/DYNAMIC_OT/
rnburn Jan 25, 2018
a8a1dad
Update opentracing-cpp dependency.
rnburn Jan 25, 2018
d0d9a12
Switch to use inline tracer configuration.
rnburn Jan 25, 2018
4b538a3
Add dynamic tracer tests.
rnburn Jan 31, 2018
c0b0595
Fix mocktracer dependencies.
rnburn Feb 2, 2018
b679ae0
Merge branch 'master' of https://github.com/envoyproxy/envoy into dyn…
rnburn Feb 2, 2018
538582e
Suppress false positive from undefined behavior sanitizer.
rnburn Feb 3, 2018
db7de0d
Merge branch 'master' of https://github.com/envoyproxy/envoy into dyn…
rnburn Feb 5, 2018
0a46919
Remove code to find mocktracer library.
rnburn Feb 5, 2018
41555a1
Add test coverage that creates spans with the mocktracer.
rnburn Feb 5, 2018
af4344b
Fix prototypes.
rnburn Feb 5, 2018
caa2205
Const correctness.
rnburn Feb 5, 2018
e3bb7f5
Modify mocktracer's json format.
rnburn Feb 7, 2018
88c1deb
Merge branch 'master' of https://github.com/envoyproxy/envoy into dyn…
rnburn Feb 7, 2018
61264a9
Fix commit hashes.
rnburn Feb 7, 2018
3ede32e
Use std::make_unique.
rnburn Feb 13, 2018
3474e39
Update opentracing-cpp.
rnburn Feb 15, 2018
3f80927
Make requiresClusterName const.
rnburn Feb 21, 2018
b0d1890
Add missing test coverage.
rnburn Feb 22, 2018
e758593
Add release notes.
rnburn Feb 22, 2018
ebefe2f
Fix coverage.
rnburn Feb 22, 2018
d9e9a18
Fix formatting of error messages.
rnburn Feb 22, 2018
899ce3f
Point back to opentracing-cpp's main repository.
rnburn Feb 26, 2018
b6ca9ca
Merge branch 'master' of https://github.com/envoyproxy/envoy into dyn…
rnburn Feb 26, 2018
ace09fb
Fix format.
rnburn Feb 26, 2018
8dd4318
Add TODO note for example.
rnburn Feb 26, 2018
21c9d67
Add test coverage for setTag.
rnburn Feb 26, 2018
4567d6f
Add test coverage for LightStepLogger.
rnburn Feb 26, 2018
1b491d9
Move method to cc file.
rnburn Feb 27, 2018
8f7fa81
Fix typo.
rnburn Feb 27, 2018
5f2d430
Fix tests for lightstep_tracer_impl.
rnburn Feb 27, 2018
f1a158c
Fix build.
rnburn Feb 27, 2018
6878e86
Add missing test coverage.
rnburn Feb 27, 2018
fca28e5
Merge branch 'master' of https://github.com/envoyproxy/envoy into dyn…
rnburn Feb 27, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ REPOSITORY_LOCATIONS = dict(
remote = "https://github.com/grpc/grpc.git",
),
io_opentracing_cpp = dict(
commit = "e57161e2a4bd1f9d3a8d3edf23185f033bb45f17",
remote = "https://github.com/opentracing/opentracing-cpp", # v1.2.0
commit = "1368b7de0343388a0e217643481ec4d475ed22e5",
remote = "https://github.com/rnburn/opentracing-cpp",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know when opentracing/opentracing-cpp#45 will merge?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, I assume that lands first and we update this before landing it. Sound right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping to get it merged in this week.

),
com_lightstep_tracer_cpp = dict(
commit = "6a198acd328f976984699f7272bbec7c8b220f65",
Expand Down
2 changes: 2 additions & 0 deletions source/common/config/well_known_names.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ class HttpTracerNameValues {
const std::string LIGHTSTEP = "envoy.lightstep";
// Zipkin tracer
const std::string ZIPKIN = "envoy.zipkin";
// Dynamic tracer
const std::string DYNAMIC_OT = "envoy.dynamic.ot";
};

typedef ConstSingleton<HttpTracerNameValues> HttpTracerNames;
Expand Down
14 changes: 14 additions & 0 deletions source/common/tracing/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,20 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "dynamic_opentracing_driver_lib",
srcs = [
"dynamic_opentracing_driver_impl.cc",
],
hdrs = [
"dynamic_opentracing_driver_impl.h",
],
deps = [
":http_tracer_lib",
":opentracing_driver_lib",
],
)

envoy_cc_library(
name = "lightstep_tracer_lib",
srcs = [
Expand Down
38 changes: 38 additions & 0 deletions source/common/tracing/dynamic_opentracing_driver_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
#include "common/tracing/dynamic_opentracing_driver_impl.h"

#include "common/common/assert.h"

namespace Envoy {
namespace Tracing {

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

std::string error_message;
opentracing::expected<opentracing::DynamicTracingLibraryHandle> library_handle_maybe =
opentracing::DynamicallyLoadTracingLibrary(library.c_str(), error_message);
if (!library_handle_maybe) {
if (error_message.empty()) {
throw EnvoyException{fmt::format("{}", library_handle_maybe.error().message())};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to test these cases, here and below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I simplified so that there's only a single code path now that should get hit in the testing.

} else {
throw EnvoyException{
fmt::format("{}: {}", library_handle_maybe.error().message(), error_message)};
}
}
library_handle_ = std::move(*library_handle_maybe);

opentracing::expected<std::shared_ptr<opentracing::Tracer>> tracer_maybe =
library_handle_.tracer_factory().MakeTracer(tracer_config.c_str(), error_message);
if (!tracer_maybe) {
if (error_message.empty()) {
throw EnvoyException{fmt::format("{}", tracer_maybe.error().message())};
} else {
throw EnvoyException{fmt::format("{}: {}", tracer_maybe.error().message(), error_message)};
}
}
tracer_ = std::move(*tracer_maybe);
RELEASE_ASSERT(tracer_ != nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this RELEASE_ASSERT and not ASSERT?

}

} // namespace Tracing
} // namespace Envoy
37 changes: 37 additions & 0 deletions source/common/tracing/dynamic_opentracing_driver_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#pragma once

#include "envoy/runtime/runtime.h"
#include "envoy/thread_local/thread_local.h"
#include "envoy/tracing/http_tracer.h"
#include "envoy/upstream/cluster_manager.h"

#include "common/tracing/opentracing_driver_impl.h"

#include "opentracing/dynamic_load.h"

namespace Envoy {
namespace Tracing {

/**
* This driver provides support for dynamically loading tracing libraries into Envoy that provide an
* implementation of the OpenTracing API (see https://github.com/opentracing/opentracing-cpp).
*/
class DynamicOpenTracingDriver : public OpenTracingDriver {
public:
DynamicOpenTracingDriver(Stats::Store& stats, const std::string& library,
const std::string& tracer_config);

// Tracer::OpenTracingDriver
opentracing::Tracer& tracer() override { return *tracer_; }

PropagationMode propagationMode() const override {
return OpenTracingDriver::PropagationMode::TracerNative;
}

private:
opentracing::DynamicTracingLibraryHandle library_handle_;
std::shared_ptr<opentracing::Tracer> tracer_;
};

} // namespace Tracing
} // namespace Envoy
4 changes: 2 additions & 2 deletions source/common/tracing/lightstep_tracer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ LightStepDriver::TlsLightStepTracer::TlsLightStepTracer(
enableTimer();
}

const opentracing::Tracer& LightStepDriver::TlsLightStepTracer::tracer() const { return *tracer_; }
opentracing::Tracer& LightStepDriver::TlsLightStepTracer::tracer() { return *tracer_; }

void LightStepDriver::TlsLightStepTracer::enableTimer() {
const uint64_t flush_interval =
Expand Down Expand Up @@ -160,7 +160,7 @@ LightStepDriver::LightStepDriver(const Json::Object& config,
});
}

const opentracing::Tracer& LightStepDriver::tracer() const {
opentracing::Tracer& LightStepDriver::tracer() {
return tls_->getTyped<TlsLightStepTracer>().tracer();
}

Expand Down
4 changes: 2 additions & 2 deletions source/common/tracing/lightstep_tracer_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class LightStepDriver : public OpenTracingDriver {
LightstepTracerStats& tracerStats() { return tracer_stats_; }

// Tracer::OpenTracingDriver
const opentracing::Tracer& tracer() const override;
opentracing::Tracer& tracer() override;
PropagationMode propagationMode() const override { return propagation_mode_; }

private:
Expand Down Expand Up @@ -90,7 +90,7 @@ class LightStepDriver : public OpenTracingDriver {
TlsLightStepTracer(const std::shared_ptr<lightstep::LightStepTracer>& tracer,
LightStepDriver& driver, Event::Dispatcher& dispatcher);

const opentracing::Tracer& tracer() const;
opentracing::Tracer& tracer();

private:
void enableTimer();
Expand Down
2 changes: 1 addition & 1 deletion source/common/tracing/opentracing_driver_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class OpenTracingDriver : public Driver, protected Logger::Loggable<Logger::Id::
SpanPtr startSpan(const Config& config, Http::HeaderMap& request_headers,
const std::string& operation_name, SystemTime start_time) override;

virtual const opentracing::Tracer& tracer() const PURE;
virtual opentracing::Tracer& tracer() PURE;

enum class PropagationMode { SingleHeader, TracerNative };

Expand Down
1 change: 1 addition & 0 deletions source/exe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ envoy_cc_library(
"//source/server:hot_restart_lib",
"//source/server:options_lib",
"//source/server/config/http:lightstep_lib",
"//source/server/config/http:dynamic_opentracing_lib",
"//source/server/config/http:zipkin_lib",
] + select({
"//bazel:disable_signal_trace": [],
Expand Down
12 changes: 12 additions & 0 deletions source/server/config/http/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,18 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "dynamic_opentracing_lib",
srcs = ["dynamic_opentracing_http_tracer.cc"],
hdrs = ["dynamic_opentracing_http_tracer.h"],
deps = [
"//source/common/config:well_known_names",
"//source/common/tracing:dynamic_opentracing_driver_lib",
"//source/common/tracing:http_tracer_lib",
"//source/server:configuration_lib",
],
)

envoy_cc_library(
name = "ratelimit_lib",
srcs = ["ratelimit.cc"],
Expand Down
37 changes: 37 additions & 0 deletions source/server/config/http/dynamic_opentracing_http_tracer.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#include "server/config/http/dynamic_opentracing_http_tracer.h"

#include <string>

#include "envoy/registry/registry.h"

#include "common/common/utility.h"
#include "common/config/well_known_names.h"
#include "common/tracing/dynamic_opentracing_driver_impl.h"
#include "common/tracing/http_tracer_impl.h"

namespace Envoy {
namespace Server {
namespace Configuration {

Tracing::HttpTracerPtr DynamicOpenTracingHttpTracerFactory::createHttpTracer(
const Json::Object& json_config, Server::Instance& server,
Upstream::ClusterManager& /*cluster_manager*/) {
const std::string library = json_config.getString("library");
const std::string config = json_config.getObject("config")->asJsonString();
Tracing::DriverPtr dynamic_driver{
std::make_unique<Tracing::DynamicOpenTracingDriver>(server.stats(), library, config)};
return std::make_unique<Tracing::HttpTracerImpl>(std::move(dynamic_driver), server.localInfo());
}

std::string DynamicOpenTracingHttpTracerFactory::name() {
return Config::HttpTracerNames::get().DYNAMIC_OT;
}

/**
* Static registration for the dynamic opentracing http tracer. @see RegisterFactory.
*/
static Registry::RegisterFactory<DynamicOpenTracingHttpTracerFactory, HttpTracerFactory> register_;

} // namespace Configuration
} // namespace Server
} // namespace Envoy
29 changes: 29 additions & 0 deletions source/server/config/http/dynamic_opentracing_http_tracer.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#pragma once

#include <string>

#include "envoy/server/instance.h"

#include "server/configuration_impl.h"

namespace Envoy {
namespace Server {
namespace Configuration {

/**
* Config registration for the dynamic opentracing tracer. @see HttpTracerFactory.
*/
class DynamicOpenTracingHttpTracerFactory : public HttpTracerFactory {
public:
// HttpTracerFactory
Tracing::HttpTracerPtr createHttpTracer(const Json::Object& json_config, Server::Instance& server,
Upstream::ClusterManager& cluster_manager) override;

std::string name() override;

bool requiresClusterName() override { return false; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be unused/ untested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test coverage.

};

} // namespace Configuration
} // namespace Server
} // namespace Envoy
10 changes: 5 additions & 5 deletions source/server/configuration_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,6 @@ void MainImpl::initializeTracers(const envoy::config::trace::v2::Tracing& config
return;
}

if (server.localInfo().clusterName().empty()) {
throw EnvoyException("cluster name must be defined if tracing is enabled. See "
"--service-cluster option.");
}

// Initialize tracing driver.
std::string type = configuration.http().name();
ENVOY_LOG(info, " loading tracing driver: {}", type);
Expand All @@ -111,6 +106,11 @@ void MainImpl::initializeTracers(const envoy::config::trace::v2::Tracing& config

// Now see if there is a factory that will accept the config.
auto& factory = Config::Utility::getAndCheckFactory<HttpTracerFactory>(type);
if (factory.requiresClusterName() && server.localInfo().clusterName().empty()) {
throw EnvoyException(fmt::format("cluster name must be defined for the tracing driver {}. See "
"--service-cluster option.",
type));
}
http_tracer_ = factory.createHttpTracer(*driver_config, server, *cluster_manager_);
}

Expand Down
5 changes: 5 additions & 0 deletions source/server/configuration_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ class HttpTracerFactory {
* factory.
*/
virtual std::string name() PURE;

/**
* Returns true if the tracing driver requires cluster name to be defined.
*/
virtual bool requiresClusterName() { return true; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be a const method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

};

/**
Expand Down
18 changes: 18 additions & 0 deletions test/common/tracing/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,21 @@ envoy_cc_test(
"//test/test_common:utility_lib",
],
)

envoy_cc_test(
name = "dynamic_opentracing_driver_impl_test",
srcs = [
"dynamic_opentracing_driver_impl_test.cc",
],
data = [
"@io_opentracing_cpp//mocktracer:libmocktracer_plugin.so",
],
deps = [
"//source/common/http:header_map_lib",
"//source/common/tracing:dynamic_opentracing_driver_lib",
"//test/mocks/http:http_mocks",
"//test/mocks/stats:stats_mocks",
"//test/mocks/tracing:tracing_mocks",
"//test/test_common:environment_lib",
],
)
77 changes: 77 additions & 0 deletions test/common/tracing/dynamic_opentracing_driver_impl_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
#include "common/http/header_map_impl.h"
#include "common/tracing/dynamic_opentracing_driver_impl.h"

#include "test/mocks/http/mocks.h"
#include "test/mocks/stats/mocks.h"
#include "test/mocks/tracing/mocks.h"
#include "test/test_common/environment.h"

#include "fmt/printf.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"

using testing::Test;

namespace Envoy {
namespace Tracing {

class DynamicOpenTracingDriverTest : public Test {
public:
void setup(const std::string& library, const std::string& tracer_config) {
driver_.reset(new DynamicOpenTracingDriver{stats_, library, tracer_config});
}

void setupValidDriver() { setup(library_path_, tracer_config_); }

const std::string library_path_ =
TestEnvironment::runfilesDirectory() +
"/external/io_opentracing_cpp/mocktracer/libmocktracer_plugin.so";
const std::string spans_file_ = TestEnvironment::temporaryDirectory() + "/spans.json";
const std::string tracer_config_ = fmt::sprintf(R"EOF(
{
"output_file": "%s"
}
)EOF",
spans_file_);
std::unique_ptr<DynamicOpenTracingDriver> driver_;
Stats::IsolatedStoreImpl stats_;

const std::string operation_name_{"test"};
Http::TestHeaderMapImpl request_headers_{
{":path", "/"}, {":method", "GET"}, {"x-request-id", "foo"}};
SystemTime start_time_;
NiceMock<Tracing::MockConfig> config_;
};

TEST_F(DynamicOpenTracingDriverTest, InitializeDriver) {
{
std::string invalid_library = "abc123";
std::string invalid_config = R"EOF(
{"fake" : "fake"}
)EOF";

EXPECT_THROW(setup(invalid_library, invalid_config), EnvoyException);
}

{
std::string empty_config = "{}";

EXPECT_THROW(setup(library_path_, empty_config), EnvoyException);
}
}

TEST_F(DynamicOpenTracingDriverTest, FlushSpans) {
setupValidDriver();

SpanPtr first_span = driver_->startSpan(config_, request_headers_, operation_name_, start_time_);
first_span->finishSpan();
driver_->tracer().Close();

const Json::ObjectSharedPtr spans_json =
TestEnvironment::jsonLoadFromString(TestEnvironment::readFileToStringForTest(spans_file_));
EXPECT_NE(spans_json, nullptr);
EXPECT_EQ(spans_json->asObjectArray().size(), 1);
}

} // namespace Tracing
} // namespace Envoy
Loading