-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Changes from 21 commits
3dfbfbc
4aa5578
3473a7d
c1ad0d3
5810042
a8a1dad
d0d9a12
4b538a3
c0b0595
b679ae0
538582e
db7de0d
0a46919
41555a1
af4344b
caa2205
e3bb7f5
88c1deb
61264a9
3ede32e
3474e39
3f80927
b0d1890
e758593
ebefe2f
d9e9a18
899ce3f
b6ca9ca
ace09fb
8dd4318
21c9d67
4567d6f
1b491d9
8f7fa81
5f2d430
f1a158c
6878e86
fca28e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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} { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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())}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to test these cases, here and below? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this |
||
} | ||
|
||
} // namespace Tracing | ||
} // namespace Envoy |
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 |
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 |
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; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems to be unused/ untested? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added test coverage. |
||
}; | ||
|
||
} // namespace Configuration | ||
} // namespace Server | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can be a const method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. |
||
}; | ||
|
||
/** | ||
|
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.