From b70e65cd131d16c4db81f82e7e7765a9f3071f0e Mon Sep 17 00:00:00 2001 From: Will Gittoes Date: Fri, 18 Jan 2019 16:39:21 -0500 Subject: [PATCH 1/5] Move ENV var checking to apply in all cases, add flags for enabling/disabling B3 header propagation --- src/opentracing_agent.cpp | 18 +++++++-- src/opentracing_external.cpp | 14 ++++++- src/tracer_factory.cpp | 42 +++---------------- src/tracer_options.cpp | 57 ++++++++++++++++++++++++++ src/tracer_options.h | 36 +++++++++++++++++ test/CMakeLists.txt | 5 ++- test/tracer_options_test.cpp | 78 ++++++++++++++++++++++++++++++++++++ 7 files changed, 206 insertions(+), 44 deletions(-) create mode 100644 src/tracer_options.cpp create mode 100644 src/tracer_options.h create mode 100644 test/tracer_options_test.cpp diff --git a/src/opentracing_agent.cpp b/src/opentracing_agent.cpp index c6b46e3d..f8462893 100644 --- a/src/opentracing_agent.cpp +++ b/src/opentracing_agent.cpp @@ -7,6 +7,7 @@ #include "agent_writer.h" #include "sample.h" #include "tracer.h" +#include "tracer_options.h" namespace ot = opentracing; @@ -14,11 +15,20 @@ namespace datadog { namespace opentracing { std::shared_ptr makeTracer(const TracerOptions &options) { - std::shared_ptr sampler = sampleProviderFromOptions(options); + auto maybe_options = applyTracerOptionsFromEnvironment(options); + if (!maybe_options) { + std::cerr << "Error applying TracerOptions from environment variables: " + << maybe_options.error() << std::endl + << "Tracer will be started without options from the environment" << std::endl; + maybe_options = options; + } + TracerOptions opts = maybe_options.value(); + + std::shared_ptr sampler = sampleProviderFromOptions(opts); auto writer = std::shared_ptr{ - new AgentWriter(options.agent_host, options.agent_port, - std::chrono::milliseconds(llabs(options.write_period_ms)), sampler)}; - return std::shared_ptr{new Tracer{options, writer, sampler}}; + new AgentWriter(opts.agent_host, opts.agent_port, + std::chrono::milliseconds(llabs(opts.write_period_ms)), sampler)}; + return std::shared_ptr{new Tracer{opts, writer, sampler}}; } } // namespace opentracing diff --git a/src/opentracing_external.cpp b/src/opentracing_external.cpp index 403eddd1..76f0296d 100644 --- a/src/opentracing_external.cpp +++ b/src/opentracing_external.cpp @@ -8,6 +8,7 @@ #include #include "sample.h" #include "tracer.h" +#include "tracer_options.h" #include "writer.h" namespace ot = opentracing; @@ -17,12 +18,21 @@ namespace opentracing { std::tuple, std::shared_ptr> makeTracerAndEncoder( const TracerOptions &options) { - std::shared_ptr sampler = sampleProviderFromOptions(options); + auto maybe_options = applyTracerOptionsFromEnvironment(options); + if (!maybe_options) { + std::cerr << "Error applying TracerOptions from environment variables: " + << maybe_options.error() << std::endl + << "Tracer will be started without options from the environment" << std::endl; + maybe_options = options; + } + TracerOptions opts = maybe_options.value(); + + std::shared_ptr sampler = sampleProviderFromOptions(opts); auto xwriter = std::make_shared(sampler); auto encoder = xwriter->encoder(); std::shared_ptr writer = xwriter; return std::tuple, std::shared_ptr>{ - std::shared_ptr{new Tracer{options, writer, sampler}}, encoder}; + std::shared_ptr{new Tracer{opts, writer, sampler}}, encoder}; } } // namespace opentracing diff --git a/src/tracer_factory.cpp b/src/tracer_factory.cpp index 8c2b81b7..d9f945ab 100644 --- a/src/tracer_factory.cpp +++ b/src/tracer_factory.cpp @@ -4,31 +4,13 @@ #include #include "agent_writer.h" #include "tracer.h" +#include "tracer_options.h" using json = nlohmann::json; namespace datadog { namespace opentracing { -namespace { -ot::expected> asPropagationStyle(json styles) { - std::set propagation_styles; - for (auto &style : styles) { - if (style == "Datadog") { - propagation_styles.insert(PropagationStyle::Datadog); - } else if (style == "B3") { - propagation_styles.insert(PropagationStyle::B3); - } else { - return ot::make_unexpected(std::make_error_code(std::errc::invalid_argument)); - } - } - if (propagation_styles.size() == 0) { - return ot::make_unexpected(std::make_error_code(std::errc::invalid_argument)); - } - return propagation_styles; -} -} // namespace - ot::expected optionsFromConfig(const char *configuration, std::string &error_message) { TracerOptions options{"localhost", 8126, "", "web", "", 1.0}; @@ -93,24 +75,12 @@ ot::expected optionsFromConfig(const char *configuration, return ot::make_unexpected(std::make_error_code(std::errc::invalid_argument)); } - // Agent host and port environment variables override defaults and config. - auto agent_host = std::getenv("DD_AGENT_HOST"); - if (agent_host != nullptr && std::strlen(agent_host) > 0) { - options.agent_host = agent_host; - } - auto trace_agent_port = std::getenv("DD_TRACE_AGENT_PORT"); - if (trace_agent_port != nullptr && std::strlen(trace_agent_port) > 0) { - try { - options.agent_port = std::stoi(trace_agent_port); - } catch (const std::invalid_argument &ia) { - error_message = "Value for DD_TRACE_AGENT_PORT is invalid"; - return ot::make_unexpected(std::make_error_code(std::errc::invalid_argument)); - } catch (const std::out_of_range &oor) { - error_message = "Value for DD_TRACE_AGENT_PORT is out of range"; - return ot::make_unexpected(std::make_error_code(std::errc::invalid_argument)); - } + auto maybe_options = applyTracerOptionsFromEnvironment(options); + if (!maybe_options) { + error_message = maybe_options.error(); + return ot::make_unexpected(std::make_error_code(std::errc::invalid_argument)); } - return options; + return maybe_options.value(); } // Accepts configuration in JSON format, with the following keys: diff --git a/src/tracer_options.cpp b/src/tracer_options.cpp new file mode 100644 index 00000000..5bf8dc94 --- /dev/null +++ b/src/tracer_options.cpp @@ -0,0 +1,57 @@ +#include "tracer_options.h" + +#include +#include + +namespace ot = opentracing; + +namespace datadog { +namespace opentracing { + +ot::expected applyTracerOptionsFromEnvironment( + const TracerOptions &input) { + TracerOptions opts = input; + + auto agent_host = std::getenv("DD_AGENT_HOST"); + if (agent_host != nullptr && std::strlen(agent_host) > 0) { + opts.agent_host = agent_host; + } + + auto trace_agent_port = std::getenv("DD_TRACE_AGENT_PORT"); + if (trace_agent_port != nullptr && std::strlen(trace_agent_port) > 0) { + try { + opts.agent_port = std::stoi(trace_agent_port); + } catch (const std::invalid_argument &ia) { + return ot::make_unexpected("Value for DD_TRACE_AGENT_PORT is invalid"); + } catch (const std::out_of_range &oor) { + return ot::make_unexpected("Value for DD_TRACE_AGENT_PORT is out of range"); + } + } + + auto extract = std::getenv("DD_PROPAGATION_STYLE_EXTRACT"); + if (extract != nullptr && std::strlen(extract) > 0) { + std::stringstream words{extract}; + auto style_maybe = asPropagationStyle(std::vector{ + std::istream_iterator{words}, std::istream_iterator{}}); + if (!style_maybe) { + return ot::make_unexpected("Value for DD_PROPAGATION_STYLE_EXTRACT is invalid"); + } + opts.extract = style_maybe.value(); + } + + auto inject = std::getenv("DD_PROPAGATION_STYLE_INJECT"); + if (inject != nullptr && std::strlen(inject) > 0) { + std::stringstream words{inject}; + auto style_maybe = asPropagationStyle(std::vector{ + std::istream_iterator{words}, std::istream_iterator{}}); + if (!style_maybe) { + return ot::make_unexpected("Value for DD_PROPAGATION_STYLE_INJECT is invalid"); + } + opts.inject = style_maybe.value(); + } + + return opts; +} + +} // namespace opentracing +} // namespace datadog diff --git a/src/tracer_options.h b/src/tracer_options.h new file mode 100644 index 00000000..b1096dfd --- /dev/null +++ b/src/tracer_options.h @@ -0,0 +1,36 @@ +#ifndef DD_TRACER_OPTIONS_H +#define DD_TRACER_OPTIONS_H + +#include +#include + +namespace ot = opentracing; + +namespace datadog { +namespace opentracing { + +template +ot::expected> asPropagationStyle(Iterable styles) { + std::set propagation_styles; + for (const std::string& style : styles) { + if (style == "Datadog") { + propagation_styles.insert(PropagationStyle::Datadog); + } else if (style == "B3") { + propagation_styles.insert(PropagationStyle::B3); + } else { + return ot::make_unexpected(std::make_error_code(std::errc::invalid_argument)); + } + } + if (propagation_styles.size() == 0) { + return ot::make_unexpected(std::make_error_code(std::errc::invalid_argument)); + } + return propagation_styles; +}; + +ot::expected applyTracerOptionsFromEnvironment( + const TracerOptions& input); + +} // namespace opentracing +} // namespace datadog + +#endif // DD_TRACER_OPTIONS_H diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index e31da9cb..250b5a28 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -6,11 +6,12 @@ macro(_datadog_test TEST_NAME) add_test(${TEST_NAME} ${TEST_NAME}) endmacro() +_datadog_test(agent_writer_test agent_writer_test.cpp) _datadog_test(opentracing_test opentracing_test.cpp) _datadog_test(propagation_test propagation_test.cpp) +_datadog_test(sample_test sample_test.cpp) _datadog_test(span_buffer_test span_buffer_test.cpp) _datadog_test(span_test span_test.cpp) _datadog_test(tracer_factory_test tracer_factory_test.cpp) +_datadog_test(tracer_options_test tracer_options_test.cpp) _datadog_test(tracer_test tracer_test.cpp) -_datadog_test(sample_test sample_test.cpp) -_datadog_test(agent_writer_test agent_writer_test.cpp) diff --git a/test/tracer_options_test.cpp b/test/tracer_options_test.cpp new file mode 100644 index 00000000..8c0a28c7 --- /dev/null +++ b/test/tracer_options_test.cpp @@ -0,0 +1,78 @@ +#include "../src/tracer_options.h" + +#define CATCH_CONFIG_MAIN +#include +using namespace datadog::opentracing; + +void requireTracerOptionsResultsMatch(const ot::expected &lhs, + const ot::expected &rhs) { + // One is an error, the other not. + REQUIRE((!!lhs) == (!!rhs)); + // They're both errors. + if (!lhs) { + REQUIRE(std::string(lhs.error()) == std::string(rhs.error())); + return; + } + // Did someone add a field to TracerOptions and forget to update this test? + REQUIRE(sizeof(TracerOptions) == 200); + // Compare TracerOptions. + REQUIRE(lhs->agent_host == rhs->agent_host); + REQUIRE(lhs->agent_port == rhs->agent_port); + REQUIRE(lhs->service == rhs->service); + REQUIRE(lhs->type == rhs->type); + REQUIRE(lhs->environment == rhs->environment); + REQUIRE(lhs->sample_rate == rhs->sample_rate); + REQUIRE(lhs->priority_sampling == rhs->priority_sampling); + REQUIRE(lhs->write_period_ms == rhs->write_period_ms); + REQUIRE(lhs->operation_name_override == rhs->operation_name_override); + REQUIRE(lhs->extract == rhs->extract); + REQUIRE(lhs->inject == rhs->inject); +}; + +TEST_CASE("tracer options from environment variables") { + TracerOptions input{}; + struct TestCase { + std::map environment_variables; + ot::expected expected; + }; + + auto test_case = GENERATE(values({ + {{}, TracerOptions{}}, + {{{"DD_AGENT_HOST", "host"}, + {"DD_TRACE_AGENT_PORT", "420"}, + {"DD_PROPAGATION_STYLE_EXTRACT", "B3 Datadog"}, + {"DD_PROPAGATION_STYLE_INJECT", "Datadog B3"}}, + TracerOptions{"host", + 420, + "", + "web", + "", + 1.0, + true, + 1000, + "", + {PropagationStyle::Datadog, PropagationStyle::B3}, + {PropagationStyle::Datadog, PropagationStyle::B3}}}, + {{{"DD_PROPAGATION_STYLE_EXTRACT", "Not even a real style"}}, + ot::make_unexpected("Value for DD_PROPAGATION_STYLE_EXTRACT is invalid")}, + {{{"DD_PROPAGATION_STYLE_INJECT", "Not even a real style"}}, + ot::make_unexpected("Value for DD_PROPAGATION_STYLE_INJECT is invalid")}, + {{{"DD_TRACE_AGENT_PORT", "sixty nine"}}, + ot::make_unexpected("Value for DD_TRACE_AGENT_PORT is invalid")}, + {{{"DD_TRACE_AGENT_PORT", "9223372036854775807"}}, + ot::make_unexpected("Value for DD_TRACE_AGENT_PORT is out of range")}, + })); + + // Setup + for (const auto &env_var : test_case.environment_variables) { + REQUIRE(setenv(env_var.first.c_str(), env_var.second.c_str(), 1) == 0); + } + + auto got = applyTracerOptionsFromEnvironment(input); + requireTracerOptionsResultsMatch(test_case.expected, got); + + // Teardown + for (const auto &env_var : test_case.environment_variables) { + REQUIRE(unsetenv(env_var.first.c_str()) == 0); + } +} From 427a40dbb0bca8b7c949f41d9fad3e2cc3c6cff2 Mon Sep 17 00:00:00 2001 From: Will Gittoes Date: Fri, 18 Jan 2019 16:40:50 -0500 Subject: [PATCH 2/5] Change default header style to Datadog only --- include/datadog/opentracing.h | 2 +- src/tracer_factory.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/datadog/opentracing.h b/include/datadog/opentracing.h index 9e40cd7b..8887a58b 100644 --- a/include/datadog/opentracing.h +++ b/include/datadog/opentracing.h @@ -50,7 +50,7 @@ struct TracerOptions { // is recorded in the tag "operation"). std::string operation_name_override = ""; // The style of propagation headers to accept/extract. - std::set extract{PropagationStyle::Datadog, PropagationStyle::B3}; + std::set extract{PropagationStyle::Datadog}; // The style of propagation headers to emit/inject. std::set inject{PropagationStyle::Datadog}; }; diff --git a/src/tracer_factory.cpp b/src/tracer_factory.cpp index d9f945ab..6432ab86 100644 --- a/src/tracer_factory.cpp +++ b/src/tracer_factory.cpp @@ -97,7 +97,7 @@ ot::expected optionsFromConfig(const char *configuration, // "operation_name_override": A string, if not empty it overrides the operation name (and the // overridden operation name is recorded in the tag "operation"). // "propagation_style_extract": A list of strings, each string is one of "Datadog", "B3". Defaults -// to ["Datadog", "B3"]. The type of headers to use to propagate distributed traces. +// to ["Datadog"]. The type of headers to use to propagate distributed traces. // "propagation_style_inject": A list of strings, each string is one of "Datadog", "B3". Defaults // to ["Datadog"]. The type of headers to use to receive distributed traces. // From 823470b4a216cfa4f352158278d7efde01fca9c4 Mon Sep 17 00:00:00 2001 From: Will Gittoes Date: Fri, 18 Jan 2019 16:50:27 -0500 Subject: [PATCH 3/5] Add comments about the env vars --- include/datadog/opentracing.h | 12 ++++++++---- src/tracer_factory.cpp | 12 ++++++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/include/datadog/opentracing.h b/include/datadog/opentracing.h index 8887a58b..de180a27 100644 --- a/include/datadog/opentracing.h +++ b/include/datadog/opentracing.h @@ -22,9 +22,11 @@ enum class PropagationStyle { }; struct TracerOptions { - // Hostname or IP address of the Datadog agent. + // Hostname or IP address of the Datadog agent. Can also be set by the environment variable + // DD_AGENT_HOST. std::string agent_host = "localhost"; - // Port on which the Datadog agent is running. + // Port on which the Datadog agent is running. Can also be set by the environment variable + // DD_TRACE_AGENT_PORT uint32_t agent_port = 8126; // The name of the service being traced. // See: @@ -49,9 +51,11 @@ struct TracerOptions { // If not empty, the given string overrides the operation name (and the overridden operation name // is recorded in the tag "operation"). std::string operation_name_override = ""; - // The style of propagation headers to accept/extract. + // The style of propagation headers to accept/extract. Can also be set by the environment + // variable DD_PROPAGATION_STYLE_EXTRACT. std::set extract{PropagationStyle::Datadog}; - // The style of propagation headers to emit/inject. + // The style of propagation headers to emit/inject. Can also be set by the environment variable + // DD_PROPAGATION_STYLE_INJECT. std::set inject{PropagationStyle::Datadog}; }; diff --git a/src/tracer_factory.cpp b/src/tracer_factory.cpp index 6432ab86..5c8421d7 100644 --- a/src/tracer_factory.cpp +++ b/src/tracer_factory.cpp @@ -85,8 +85,10 @@ ot::expected optionsFromConfig(const char *configuration, // Accepts configuration in JSON format, with the following keys: // "service": Required. A string, the name of the service. -// "agent_host": A string, defaults to localhost. -// "agent_port": A number, defaults to 8126. +// "agent_host": A string, defaults to localhost. Can also be set by the environment variable +// DD_AGENT_HOST +// "agent_port": A number, defaults to 8126. "type": A string, defaults to web. Can also be set by +// the environment variable DD_TRACE_AGENT_PORT // "type": A string, defaults to web. // "environment": A string, defaults to "". The environment this trace belongs to. // eg. "" (env:none), "staging", "prod" @@ -97,9 +99,11 @@ ot::expected optionsFromConfig(const char *configuration, // "operation_name_override": A string, if not empty it overrides the operation name (and the // overridden operation name is recorded in the tag "operation"). // "propagation_style_extract": A list of strings, each string is one of "Datadog", "B3". Defaults -// to ["Datadog"]. The type of headers to use to propagate distributed traces. +// to ["Datadog"]. The type of headers to use to propagate distributed traces. Can also be set +// by the environment variable DD_PROPAGATION_STYLE_EXTRACT. // "propagation_style_inject": A list of strings, each string is one of "Datadog", "B3". Defaults -// to ["Datadog"]. The type of headers to use to receive distributed traces. +// to ["Datadog"]. The type of headers to use to receive distributed traces. Can also be set by +// the environment variable DD_PROPAGATION_STYLE_INJECT. // // Extra keys will be ignored. template From 433448eeab9e896bd0d04c80e291beb35c0913e6 Mon Sep 17 00:00:00 2001 From: Will Gittoes Date: Fri, 18 Jan 2019 16:58:03 -0500 Subject: [PATCH 4/5] Remove ill-advised test --- test/tracer_options_test.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/tracer_options_test.cpp b/test/tracer_options_test.cpp index 8c0a28c7..f6a57bbc 100644 --- a/test/tracer_options_test.cpp +++ b/test/tracer_options_test.cpp @@ -13,8 +13,6 @@ void requireTracerOptionsResultsMatch(const ot::expectedagent_host == rhs->agent_host); REQUIRE(lhs->agent_port == rhs->agent_port); From 29f43ea18aca7beecf2525dbe30e28aca69cf1bb Mon Sep 17 00:00:00 2001 From: Will Gittoes Date: Fri, 18 Jan 2019 17:06:42 -0500 Subject: [PATCH 5/5] Update bazel --- BUILD.bazel | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/BUILD.bazel b/BUILD.bazel index 26ffee8d..342e7166 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -2,21 +2,23 @@ cc_library( name = "dd_opentracing_cpp", srcs = [ "src/clock.h", + "src/encoder.cpp", + "src/encoder.h", "src/noopspan.cpp", "src/noopspan.h", "src/opentracing_external.cpp", "src/propagation.cpp", "src/propagation.h", - "src/encoder.cpp", - "src/encoder.h", "src/sample.cpp", "src/sample.h", - "src/span_buffer.cpp", - "src/span_buffer.h", "src/span.cpp", "src/span.h", + "src/span_buffer.cpp", + "src/span_buffer.h", "src/tracer.cpp", "src/tracer.h", + "src/tracer_options.cpp", + "src/tracer_options.h", "src/writer.cpp", "src/writer.h", ":version_number.h", @@ -24,13 +26,6 @@ cc_library( hdrs = [ "include/datadog/opentracing.h", ], - strip_include_prefix = "include", - visibility = ["//visibility:public"], - deps = [ - "//:3rd_party_nlohmann", - "@io_opentracing_cpp//:opentracing", - "@com_github_msgpack_msgpack_c//:msgpack", - ], copts = [ "-Wall", "-Wextra", @@ -40,16 +35,23 @@ cc_library( "-Wold-style-cast", "-std=c++14", ], + strip_include_prefix = "include", + visibility = ["//visibility:public"], + deps = [ + "//:3rd_party_nlohmann", + "@com_github_msgpack_msgpack_c//:msgpack", + "@io_opentracing_cpp//:opentracing", + ], ) genrule( name = "generate_version_number_h", srcs = glob([ - "CMakeLists.txt", - "src/*", + "CMakeLists.txt", + "src/*", ]), outs = [ - "version_number.h" + "version_number.h", ], cmd = """ TEMP_DIR=$$(mktemp -d)