From 394c3686c0f19a028c08d0f8e92abc12aa3695f1 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sun, 7 Jan 2024 23:08:45 -0500 Subject: [PATCH 01/19] Get bazel to build the library on Windows. - `T(T&) = default;` is not a thing. - That's neither a copy constructor nor a move constructor. - In order to fix this, it was best to remove the template flavors of `Expected` member functions. Instead, I manually define the relevant overloads. - `#include ` before including other Windows-related headers. - Use `.bazelrc` for platform-specific compiler options. - But now how will Envoy know to `-DDD_USE_ABSEIL_FOR_ENVOY`? --- .bazelrc | 11 ++++++++ .gitignore | 2 ++ BUILD.bazel | 8 ------ src/datadog/expected.h | 50 ++++++++++++++++------------------- src/datadog/platform_util.cpp | 1 + 5 files changed, 37 insertions(+), 35 deletions(-) create mode 100644 .bazelrc diff --git a/.bazelrc b/.bazelrc new file mode 100644 index 00000000..90692f4a --- /dev/null +++ b/.bazelrc @@ -0,0 +1,11 @@ +# In order to support both Unixen and Windows, different styles of compiler +# flags must be used. +# +# This .bazelrc file specifies different compiler flags for Linux/Darwin versus +# Windows. + +build --enable_platform_specific_config + +build:linux --cxxopt='-std=c++17' --cxxopt='-Wall' --cxxopt='-Wextra' --cxxopt='-pedantic' --cxxopt='-DDD_USE_ABSEIL_FOR_ENVOY' +build:macos --cxxopt='-std=c++17' --cxxopt='-Wall' --cxxopt='-Wextra' --cxxopt='-pedantic' --cxxopt='-DDD_USE_ABSEIL_FOR_ENVOY' +build:windows --cxxopt='/std:c++17' --cxxopt='/DDD_USE_ABSEIL_FOR_ENVOY' diff --git a/.gitignore b/.gitignore index 61bc361c..478c3c9f 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,5 @@ /bazel-* /.build/ /.coverage/ +/MODULE.bazel +/MODULE.bazel.lock diff --git a/BUILD.bazel b/BUILD.bazel index a91d7ec6..60b1adba 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -104,14 +104,6 @@ cc_library( "src/datadog/version.h", "src/datadog/w3c_propagation.h", ], - copts = [ - "-Wall", - "-Wextra", - "-Werror", - "-pedantic", - "-std=c++17", - "-DDD_USE_ABSEIL_FOR_ENVOY", - ], strip_include_prefix = "src/", visibility = ["//visibility:public"], deps = [ diff --git a/src/datadog/expected.h b/src/datadog/expected.h index f442fc55..e82919cb 100644 --- a/src/datadog/expected.h +++ b/src/datadog/expected.h @@ -37,6 +37,7 @@ // error then it cannot be "dereferenced" with `operator*`, i.e. it is analogous // to `Optional` (and is implemented as such). +#include #include #include "error.h" @@ -52,16 +53,16 @@ class Expected { public: Expected() = default; Expected(const Expected&) = default; - Expected(Expected&) = default; Expected(Expected&&) = default; + + Expected(const Value&); + Expected(Value&&); + Expected(const Error&); + Expected(Error&&); + Expected& operator=(const Expected&) = default; Expected& operator=(Expected&&) = default; - template - Expected(Other&&); - template - Expected& operator=(Other&&); - // Return whether this object holds a `Value` (as opposed to an `Error`). bool has_value() const noexcept; explicit operator bool() const noexcept; @@ -99,15 +100,16 @@ class Expected { }; template -template -Expected::Expected(Other&& other) : data_(std::forward(other)) {} +Expected::Expected(const Value& value) : data_(value) {} template -template -Expected& Expected::operator=(Other&& other) { - data_ = std::forward(other); - return *this; -} +Expected::Expected(Value&& value) : data_(std::move(value)) {} + +template +Expected::Expected(const Error& error) : data_(error) {} + +template +Expected::Expected(Error&& error) : data_(std::move(error)) {} template bool Expected::has_value() const noexcept { @@ -194,16 +196,15 @@ class Expected { public: Expected() = default; Expected(const Expected&) = default; - Expected(Expected&) = default; Expected(Expected&&) = default; + + Expected(const Error&); + Expected(Error&&); + Expected(decltype(nullopt)); + Expected& operator=(const Expected&) = default; Expected& operator=(Expected&&) = default; - template - Expected(Other&&); - template - Expected& operator=(Other&&); - void swap(Expected& other); bool has_value() const; @@ -221,14 +222,9 @@ class Expected { const Error* if_error() const&& = delete; }; -template -Expected::Expected(Other&& other) : data_(std::forward(other)) {} - -template -Expected& Expected::operator=(Other&& other) { - data_ = std::forward(other); - return *this; -} +inline Expected::Expected(const Error& error) : data_(error) {} +inline Expected::Expected(Error&& error) : data_(std::move(error)) {} +inline Expected::Expected(decltype(nullopt)) : data_(nullopt) {} inline void Expected::swap(Expected& other) { data_.swap(other.data_); } diff --git a/src/datadog/platform_util.cpp b/src/datadog/platform_util.cpp index c7c2c8b1..5cf2be42 100644 --- a/src/datadog/platform_util.cpp +++ b/src/datadog/platform_util.cpp @@ -1,6 +1,7 @@ #include "platform_util.h" #ifdef _MSC_VER +#include #include #include #else From 6a077f86f4e16b756adf28261e0fc4def818c2d4 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sun, 7 Jan 2024 23:08:45 -0500 Subject: [PATCH 02/19] Get bazel to test the library on Windows. - Setting an environment variable to an empty string instead removes the environment variable. This affected some tests. - ::gethostname requires special setup. - timegm(...) has a different name. - The compiler wouldn't automatically capture a local `const std::time_t`. - Since bazel builds don't include libcurl, tests had to be updated to use a mock `HTTPClient` instance, otherwise configuration fails. - bazel builds use `absl::string_view`, so some appearances of `operator=` had to be changed to `assign`. - Deleting a file using the facilities in `std::filesystem` does not delete the file. - `::gethostname` requires winsock linkage. - `Expected` now does not contain any member function templates. I don't want to think about how to explain why. --- .bazelrc | 2 +- BUILD.bazel | 46 +++++++++++++++++++++++++++ src/datadog/environment.cpp | 36 ++++++++++++++++----- src/datadog/environment.h | 4 ++- src/datadog/expected.h | 5 +++ src/datadog/platform_util.cpp | 43 +++++++++++++++++++++++-- test/mocks/collectors.h | 4 +-- test/mocks/http_clients.h | 2 +- test/test_limiter.cpp | 13 ++++++++ test/test_smoke.cpp | 2 ++ test/test_span.cpp | 2 ++ test/test_span_sampler.cpp | 8 ++--- test/test_tracer.cpp | 4 ++- test/test_tracer_config.cpp | 58 +++++++++++++++++++++++----------- test/test_tracer_telemetry.cpp | 2 +- 15 files changed, 192 insertions(+), 39 deletions(-) diff --git a/.bazelrc b/.bazelrc index 90692f4a..ca2fef1e 100644 --- a/.bazelrc +++ b/.bazelrc @@ -8,4 +8,4 @@ build --enable_platform_specific_config build:linux --cxxopt='-std=c++17' --cxxopt='-Wall' --cxxopt='-Wextra' --cxxopt='-pedantic' --cxxopt='-DDD_USE_ABSEIL_FOR_ENVOY' build:macos --cxxopt='-std=c++17' --cxxopt='-Wall' --cxxopt='-Wextra' --cxxopt='-pedantic' --cxxopt='-DDD_USE_ABSEIL_FOR_ENVOY' -build:windows --cxxopt='/std:c++17' --cxxopt='/DDD_USE_ABSEIL_FOR_ENVOY' +build:windows --cxxopt='/std:c++17' --cxxopt='/DDD_USE_ABSEIL_FOR_ENVOY' --linkopt='ws2_32.lib' diff --git a/BUILD.bazel b/BUILD.bazel index 60b1adba..2b4d33be 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -111,3 +111,49 @@ cc_library( "@com_google_absl//absl/types:optional", ], ) + + +cc_test( + name = "dd_trace_cpp_test", + srcs = [ + # test driver + "test/main.cpp", + + # wrapper around Catch2 + "test/test.cpp", + + # mocks + "test/mocks/collectors.cpp", + "test/mocks/dict_readers.cpp", + "test/mocks/dict_writers.cpp", + "test/mocks/event_schedulers.cpp", + "test/mocks/http_clients.cpp", + "test/mocks/loggers.cpp", + + # utilities + "test/matchers.cpp", + + # test cases + "test/test_cerr_logger.cpp", + # "test/test_curl.cpp", no curl: bazel build/test is for Envoy. + "test/test_datadog_agent.cpp", + "test/test_glob.cpp", + "test/test_limiter.cpp", + "test/test_metrics.cpp", + "test/test_msgpack.cpp", + "test/test_parse_util.cpp", + "test/test_smoke.cpp", + "test/test_span.cpp", + "test/test_span_sampler.cpp", + "test/test_trace_id.cpp", + "test/test_trace_segment.cpp", + "test/test_tracer_config.cpp", + "test/test_tracer_telemetry.cpp", + "test/test_tracer.cpp", + "test/test_trace_sampler.cpp", + ], + includes = ["test"], + deps = [ + "dd_trace_cpp", + ], +) diff --git a/src/datadog/environment.cpp b/src/datadog/environment.cpp index 780bb42b..24a38963 100644 --- a/src/datadog/environment.cpp +++ b/src/datadog/environment.cpp @@ -1,30 +1,50 @@ #include "environment.h" #include +#ifdef _MSC_VER +#include +#include // GetEnvironmentVariable +#endif #include "json.hpp" namespace datadog { namespace tracing { namespace environment { +namespace { + +Optional get_env(const char *name) { +#ifdef _MSC_VER + // maximum size of an environment variable value on Windows + char buffer[32767]; + const DWORD rc = GetEnvironmentVariable(name, buffer, sizeof buffer); + if (rc == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND) { + return nullopt; + } + return std::string(buffer, rc); // `rc` is the length on success +#else + if (const char *value = std::getenv(name)) { + return value; + } + return nullopt; +#endif +} + +} // namespace StringView name(Variable variable) { return variable_names[variable]; } -Optional lookup(Variable variable) { +Optional lookup(Variable variable) { const char *name = variable_names[variable]; - const char *value = std::getenv(name); - if (!value) { - return nullopt; - } - return StringView{value}; + return get_env(name); } nlohmann::json to_json() { auto result = nlohmann::json::object({}); for (const char *name : variable_names) { - if (const char *value = std::getenv(name)) { - result[name] = value; + if (auto value = get_env(name)) { + result[name] = std::move(*value); } } diff --git a/src/datadog/environment.h b/src/datadog/environment.h index f8800d8c..f9790172 100644 --- a/src/datadog/environment.h +++ b/src/datadog/environment.h @@ -18,6 +18,8 @@ #include "optional.h" #include "string_view.h" +#include + namespace datadog { namespace tracing { namespace environment { @@ -74,7 +76,7 @@ StringView name(Variable variable); // Return the value of the specified environment `variable`, or return // `nullopt` if that variable is not set in the environment. -Optional lookup(Variable variable); +Optional lookup(Variable variable); nlohmann::json to_json(); diff --git a/src/datadog/expected.h b/src/datadog/expected.h index e82919cb..e8abf782 100644 --- a/src/datadog/expected.h +++ b/src/datadog/expected.h @@ -201,6 +201,8 @@ class Expected { Expected(const Error&); Expected(Error&&); Expected(decltype(nullopt)); + explicit Expected(const Optional&); + explicit Expected(Optional&&); Expected& operator=(const Expected&) = default; Expected& operator=(Expected&&) = default; @@ -225,6 +227,9 @@ class Expected { inline Expected::Expected(const Error& error) : data_(error) {} inline Expected::Expected(Error&& error) : data_(std::move(error)) {} inline Expected::Expected(decltype(nullopt)) : data_(nullopt) {} +inline Expected::Expected(const Optional& data) : data_(data) {} +inline Expected::Expected(Optional&& data) + : data_(std::move(data)) {} inline void Expected::swap(Expected& other) { data_.swap(other.data_); } diff --git a/src/datadog/platform_util.cpp b/src/datadog/platform_util.cpp index 5cf2be42..81194182 100644 --- a/src/datadog/platform_util.cpp +++ b/src/datadog/platform_util.cpp @@ -1,9 +1,8 @@ #include "platform_util.h" #ifdef _MSC_VER -#include +#include #include -#include #else #include #include @@ -11,8 +10,48 @@ namespace datadog { namespace tracing { +namespace { + +// On Windows, `::gethostname()` requires that the winsock library is runtime +// initialized already. +// +// `void init_winsock_if_not_already()` initializes a static `class +// WinsockLibraryGuard` instance. Because it's `static`, the instance will be +// initialized at most once. +// +// `class WinsockLibraryGuard` initializes winsock in its constructor and +// cleans up winsock in its destructor. +// See . +#ifdef _MSC_VER +class WinsockLibraryGuard { + int startup_rc_; + +public: + WinsockLibraryGuard() { + const WORD version_requested = MAKEWORD(2, 2); + WSADATA info; + startup_rc_ = WSAStartup(version_requested, &info); + } + + ~WinsockLibraryGuard() { + // Call cleanup, but only if `WSAStartup` was successful. + if (startup_rc_ == 0) { + WSACleanup(); + } + } +}; + +void init_winsock_if_not_already() { + static WinsockLibraryGuard guard; + (void)guard; +} + +#endif + +} // namespace Optional get_hostname() { + init_winsock_if_not_already(); char buffer[256]; if (::gethostname(buffer, sizeof buffer)) { return nullopt; diff --git a/test/mocks/collectors.h b/test/mocks/collectors.h index ef48e3a4..3d0495a2 100644 --- a/test/mocks/collectors.h +++ b/test/mocks/collectors.h @@ -7,9 +7,9 @@ #include #include -#include #include #include +#include #include #include #include @@ -39,7 +39,7 @@ struct MockCollector : public Collector { } std::size_t span_count() const { - return std::accumulate(chunks.begin(), chunks.end(), 0, + return std::accumulate(chunks.begin(), chunks.end(), std::size_t(0), [](std::size_t total, const auto& chunk) { return total + chunk.size(); }); diff --git a/test/mocks/http_clients.h b/test/mocks/http_clients.h index ad59006c..3459ce90 100644 --- a/test/mocks/http_clients.h +++ b/test/mocks/http_clients.h @@ -50,7 +50,7 @@ struct MockHTTPClient : public HTTPClient { on_error_ = on_error; set_headers(request_headers); } - return post_error; + return Expected(post_error); } void drain(std::chrono::steady_clock::time_point /*deadline*/) override { diff --git a/test/test_limiter.cpp b/test/test_limiter.cpp index 2ffb42f8..cd1d723f 100644 --- a/test/test_limiter.cpp +++ b/test/test_limiter.cpp @@ -6,6 +6,19 @@ #include "test.h" +// The `timegm` function has a different name on Windows. +// See . +#ifdef _MSC_VER +namespace { + +std::time_t timegm(std::tm *tm) { + return _mkgmtime(tm); +} + +} // namespace +#endif + + using namespace datadog::tracing; TEST_CASE("limiter") { diff --git a/test/test_smoke.cpp b/test/test_smoke.cpp index fc94807e..4612b25d 100644 --- a/test/test_smoke.cpp +++ b/test/test_smoke.cpp @@ -3,6 +3,7 @@ #include #include +#include "mocks/http_clients.h" #include "mocks/loggers.h" #include "test.h" @@ -11,6 +12,7 @@ using namespace datadog::tracing; TEST_CASE("smoke") { TracerConfig config; config.defaults.service = "testsvc"; + config.agent.http_client = std::make_shared(); config.logger = std::make_shared(); auto maybe_config = finalize_config(config); diff --git a/test/test_span.cpp b/test/test_span.cpp index 6c91d125..107dea97 100644 --- a/test/test_span.cpp +++ b/test/test_span.cpp @@ -21,6 +21,7 @@ #include "mocks/collectors.h" #include "mocks/dict_readers.h" #include "mocks/dict_writers.h" +#include "mocks/http_clients.h" #include "mocks/loggers.h" #include "test.h" @@ -700,6 +701,7 @@ TEST_CASE("injecting W3C tracestate header") { TEST_CASE("128-bit trace ID injection") { TracerConfig config; config.defaults.service = "testsvc"; + config.agent.http_client = std::make_shared(); config.logger = std::make_shared(); config.trace_id_128_bit = true; config.injection_styles.clear(); diff --git a/test/test_span_sampler.cpp b/test/test_span_sampler.cpp index 3944309b..d3f75880 100644 --- a/test/test_span_sampler.cpp +++ b/test/test_span_sampler.cpp @@ -72,19 +72,19 @@ SpanSamplingTags span_sampling_tags(const SpanData& span) { SpanSamplerConfig::Rule by_service(StringView service) { SpanSamplerConfig::Rule rule; - rule.service = service; + assign(rule.service, service); return rule; } SpanSamplerConfig::Rule by_name(StringView name) { SpanSamplerConfig::Rule rule; - rule.name = name; + assign(rule.name, name); return rule; } SpanSamplerConfig::Rule by_resource(StringView resource) { SpanSamplerConfig::Rule rule; - rule.resource = resource; + assign(rule.resource, resource); return rule; } @@ -98,7 +98,7 @@ SpanSamplerConfig::Rule by_tags( SpanSamplerConfig::Rule by_name_and_tags( StringView name, std::unordered_map tags) { SpanSamplerConfig::Rule rule; - rule.name = name; + assign(rule.name, name); rule.tags = std::move(tags); return rule; } diff --git a/test/test_tracer.cpp b/test/test_tracer.cpp index 959c2b96..c683dc63 100644 --- a/test/test_tracer.cpp +++ b/test/test_tracer.cpp @@ -28,6 +28,7 @@ #include "mocks/collectors.h" #include "mocks/dict_readers.h" #include "mocks/dict_writers.h" +#include "mocks/http_clients.h" #include "mocks/loggers.h" #include "test.h" @@ -1018,7 +1019,7 @@ TEST_CASE("128-bit trace IDs") { // Use a clock that always returns a hard-coded `TimePoint`. // May 6, 2010 14:45:13 America/New_York const std::time_t flash_crash = 1273171513; - const Clock clock = []() { + const Clock clock = [flash_crash = flash_crash]() { TimePoint result; result.wall = std::chrono::system_clock::from_time_t(flash_crash); return result; @@ -1272,6 +1273,7 @@ TEST_CASE("heterogeneous extraction") { TracerConfig config; config.defaults.service = "testsvc"; + config.agent.http_client = std::make_shared(); config.extraction_styles = test_case.extraction_styles; config.injection_styles = test_case.injection_styles; config.logger = std::make_shared(); diff --git a/test/test_tracer_config.cpp b/test/test_tracer_config.cpp index 0cd6e7fb..d7e1c6bb 100644 --- a/test/test_tracer_config.cpp +++ b/test/test_tracer_config.cpp @@ -20,9 +20,11 @@ #include "mocks/collectors.h" #include "mocks/event_schedulers.h" +#include "mocks/http_clients.h" #include "mocks/loggers.h" #include "test.h" #ifdef _MSC_VER +#include #include // SetEnvironmentVariable #else #include // setenv, unsetenv @@ -47,10 +49,12 @@ namespace { class EnvGuard { std::string name_; Optional former_value_; + // maximum size of an environment variable value on Windows + char buffer_[32767]; public: EnvGuard(std::string name, std::string value) : name_(std::move(name)) { - const char* current = std::getenv(name_.c_str()); + const char* current = get_value(name_); if (current) { former_value_ = current; } @@ -65,9 +69,21 @@ class EnvGuard { } } + const char *get_value(const std::string& name) { +#ifdef _MSC_VER + const DWORD rc = GetEnvironmentVariable(name.c_str(), buffer_, sizeof buffer_); + if (rc == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND) { + return nullptr; + } + return buffer_; +#else + return std::getenv(name_.c_str()); +#endif + } + void set_value(const std::string& value) { #ifdef _MSC_VER - ::SetEnvironmentVariable(name_.c_str(), value.c_str()); + REQUIRE(::SetEnvironmentVariable(name_.c_str(), value.c_str())); #else const bool overwrite = true; ::setenv(name_.c_str(), value.c_str(), overwrite); @@ -76,7 +92,7 @@ class EnvGuard { void unset() { #ifdef _MSC_VER - ::SetEnvironmentVariable(name_.c_str(), NULL); + REQUIRE(::SetEnvironmentVariable(name_.c_str(), NULL)); #else ::unsetenv(name_.c_str()); #endif @@ -143,6 +159,7 @@ class SomewhatSecureTemporaryFile : public std::fstream { TEST_CASE("TracerConfig::defaults") { TracerConfig config; + config.agent.http_client = std::make_shared(); SECTION("service is required") { SECTION("empty") { @@ -192,7 +209,6 @@ TEST_CASE("TracerConfig::defaults") { }; auto test_case = GENERATE(values({ - {"empty", "", {}, nullopt}, {"missing colon", "foo", {}, Error::TAG_MISSING_SEPARATOR}, {"trailing comma", "foo:bar, baz:123,", @@ -230,6 +246,7 @@ TEST_CASE("TracerConfig::defaults") { TEST_CASE("TracerConfig::log_on_startup") { TracerConfig config; + config.agent.http_client = std::make_shared(); config.defaults.service = "testsvc"; const auto logger = std::make_shared(); config.logger = logger; @@ -291,6 +308,7 @@ TEST_CASE("TracerConfig::log_on_startup") { TEST_CASE("TracerConfig::report_traces") { TracerConfig config; + config.agent.http_client = std::make_shared(); config.defaults.service = "testsvc"; const auto collector = std::make_shared(); config.collector = collector; @@ -361,6 +379,7 @@ TEST_CASE("TracerConfig::report_traces") { TEST_CASE("TracerConfig::agent") { TracerConfig config; config.defaults.service = "testsvc"; + config.agent.http_client = std::make_shared(); SECTION("event_scheduler") { SECTION("default") { @@ -467,7 +486,6 @@ TEST_CASE("TracerConfig::agent") { // during configuration. For the purposes of configuration, any // value is accepted. {"we don't parse port", x, "bogus", x, "http", "localhost:bogus"}, - {"even empty is ok", x, "", x, "http", "localhost:"}, {"URL", x, x, "http://dd-agent:8080", "http", "dd-agent:8080"}, {"URL overrides scheme", x, x, "https://dd-agent:8080", "https", "dd-agent:8080"}, @@ -506,6 +524,7 @@ TEST_CASE("TracerConfig::agent") { TEST_CASE("TracerConfig::trace_sampler") { TracerConfig config; + config.agent.http_client = std::make_shared(); config.defaults.service = "testsvc"; SECTION("default is no rules") { @@ -589,7 +608,6 @@ TEST_CASE("TracerConfig::trace_sampler") { }; auto test_case = GENERATE(values({ - {"empty", "", {Error::INVALID_DOUBLE}}, {"nonsense", "nonsense", {Error::INVALID_DOUBLE}}, {"trailing space", "0.23 ", {Error::INVALID_DOUBLE}}, {"out of range of double", "123e9999999999", {Error::INVALID_DOUBLE}}, @@ -655,7 +673,6 @@ TEST_CASE("TracerConfig::trace_sampler") { }; auto test_case = GENERATE(values({ - {"empty", "", {Error::INVALID_DOUBLE}}, {"nonsense", "nonsense", {Error::INVALID_DOUBLE}}, {"trailing space", "23 ", {Error::INVALID_DOUBLE}}, {"out of range of double", "123e9999999999", {Error::INVALID_DOUBLE}}, @@ -771,6 +788,7 @@ TEST_CASE("TracerConfig::trace_sampler") { TEST_CASE("TracerConfig::span_sampler") { TracerConfig config; + config.agent.http_client = std::make_shared(); config.defaults.service = "testsvc"; SECTION("default is no rules") { @@ -966,13 +984,18 @@ TEST_CASE("TracerConfig::span_sampler") { SECTION("failed usage") { SECTION("unable to open") { - std::filesystem::path defunct; - { - SomewhatSecureTemporaryFile file; - REQUIRE(file.is_open()); - defunct = file.path(); - } - const EnvGuard guard{"DD_SPAN_SAMPLING_RULES_FILE", defunct.string()}; + // It's not elegant, but neither an empty path nor a path to a + // deleted file work for this test on Windows. + // + // On Windows, deleting the file doesn't delete the file, and an + // empty path deletes the environment variable rather than set the + // environment variable empty. + // + // An easy workaround is to choose a path that is very likely not on + // the file system. + const std::string invalid = "ooga/booga/booga/booga"; + + const EnvGuard guard{"DD_SPAN_SAMPLING_RULES_FILE", invalid}; auto finalized = finalize_config(config); REQUIRE(!finalized); REQUIRE(finalized.error().code == Error::SPAN_SAMPLING_RULES_FILE_IO); @@ -1001,6 +1024,7 @@ TEST_CASE("TracerConfig::span_sampler") { TEST_CASE("TracerConfig propagation styles") { TracerConfig config; + config.agent.http_client = std::make_shared(); config.defaults.service = "testsvc"; SECTION("default style is [Datadog, W3C]") { @@ -1075,7 +1099,7 @@ TEST_CASE("TracerConfig propagation styles") { }; // brevity - const auto datadog = PropagationStyle::DATADOG, + static const auto datadog = PropagationStyle::DATADOG, b3 = PropagationStyle::B3, none = PropagationStyle::NONE; // clang-format off auto test_case = GENERATE(values({ @@ -1222,6 +1246,7 @@ TEST_CASE("TracerConfig propagation styles") { TEST_CASE("configure 128-bit trace IDs") { TracerConfig config; + config.agent.http_client = std::make_shared(); config.defaults.service = "testsvc"; SECTION("defaults to true") { REQUIRE(config.trace_id_128_bit == true); } @@ -1248,7 +1273,6 @@ TEST_CASE("configure 128-bit trace IDs") { {__LINE__, "no", false}, {__LINE__, "nein", true}, {__LINE__, "0", false}, - {__LINE__, "", true}, })); // clang-format on @@ -1259,13 +1283,11 @@ TEST_CASE("configure 128-bit trace IDs") { test_case.env_value}; config.trace_id_128_bit = true; - CAPTURE(config.trace_id_128_bit); auto finalized = finalize_config(config); REQUIRE(finalized); REQUIRE(finalized->trace_id_128_bit == test_case.expected_value); config.trace_id_128_bit = false; - CAPTURE(config.trace_id_128_bit); finalized = finalize_config(config); REQUIRE(finalized); REQUIRE(finalized->trace_id_128_bit == test_case.expected_value); diff --git a/test/test_tracer_telemetry.cpp b/test/test_tracer_telemetry.cpp index 30f31bcd..a9886073 100644 --- a/test/test_tracer_telemetry.cpp +++ b/test/test_tracer_telemetry.cpp @@ -14,7 +14,7 @@ using namespace datadog::tracing; TEST_CASE("Tracer telemetry") { const std::time_t mock_time = 1672484400; - const Clock clock = []() { + const Clock clock = [mock_time = mock_time]() { TimePoint result; result.wall = std::chrono::system_clock::from_time_t(mock_time); return result; From 034fb3f4a6be4278409ac2d63cbab14757345e01 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sun, 7 Jan 2024 23:09:45 -0500 Subject: [PATCH 03/19] bin/format --- src/datadog/environment.cpp | 4 ++-- src/datadog/environment.h | 4 ++-- src/datadog/platform_util.cpp | 9 +++++---- test/test_limiter.cpp | 7 ++----- test/test_tracer_config.cpp | 12 +++++++----- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/datadog/environment.cpp b/src/datadog/environment.cpp index 24a38963..5b59baf6 100644 --- a/src/datadog/environment.cpp +++ b/src/datadog/environment.cpp @@ -2,8 +2,8 @@ #include #ifdef _MSC_VER -#include #include // GetEnvironmentVariable +#include #endif #include "json.hpp" @@ -21,7 +21,7 @@ Optional get_env(const char *name) { if (rc == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND) { return nullopt; } - return std::string(buffer, rc); // `rc` is the length on success + return std::string(buffer, rc); // `rc` is the length on success #else if (const char *value = std::getenv(name)) { return value; diff --git a/src/datadog/environment.h b/src/datadog/environment.h index f9790172..7efaa957 100644 --- a/src/datadog/environment.h +++ b/src/datadog/environment.h @@ -14,12 +14,12 @@ // // `lookup` retrieves the value of `Variable` in the environment. +#include + #include "json_fwd.hpp" #include "optional.h" #include "string_view.h" -#include - namespace datadog { namespace tracing { namespace environment { diff --git a/src/datadog/platform_util.cpp b/src/datadog/platform_util.cpp index 81194182..13a12391 100644 --- a/src/datadog/platform_util.cpp +++ b/src/datadog/platform_util.cpp @@ -1,8 +1,8 @@ #include "platform_util.h" #ifdef _MSC_VER -#include #include +#include #else #include #include @@ -21,12 +21,13 @@ namespace { // // `class WinsockLibraryGuard` initializes winsock in its constructor and // cleans up winsock in its destructor. -// See . +// See +// . #ifdef _MSC_VER class WinsockLibraryGuard { int startup_rc_; -public: + public: WinsockLibraryGuard() { const WORD version_requested = MAKEWORD(2, 2); WSADATA info; @@ -48,7 +49,7 @@ void init_winsock_if_not_already() { #endif -} // namespace +} // namespace Optional get_hostname() { init_winsock_if_not_already(); diff --git a/test/test_limiter.cpp b/test/test_limiter.cpp index cd1d723f..c040eb1d 100644 --- a/test/test_limiter.cpp +++ b/test/test_limiter.cpp @@ -11,14 +11,11 @@ #ifdef _MSC_VER namespace { -std::time_t timegm(std::tm *tm) { - return _mkgmtime(tm); -} +std::time_t timegm(std::tm *tm) { return _mkgmtime(tm); } -} // namespace +} // namespace #endif - using namespace datadog::tracing; TEST_CASE("limiter") { diff --git a/test/test_tracer_config.cpp b/test/test_tracer_config.cpp index d7e1c6bb..ca86b9c7 100644 --- a/test/test_tracer_config.cpp +++ b/test/test_tracer_config.cpp @@ -24,8 +24,8 @@ #include "mocks/loggers.h" #include "test.h" #ifdef _MSC_VER -#include #include // SetEnvironmentVariable +#include #else #include // setenv, unsetenv #endif @@ -69,9 +69,10 @@ class EnvGuard { } } - const char *get_value(const std::string& name) { + const char* get_value(const std::string& name) { #ifdef _MSC_VER - const DWORD rc = GetEnvironmentVariable(name.c_str(), buffer_, sizeof buffer_); + const DWORD rc = + GetEnvironmentVariable(name.c_str(), buffer_, sizeof buffer_); if (rc == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND) { return nullptr; } @@ -985,7 +986,7 @@ TEST_CASE("TracerConfig::span_sampler") { SECTION("failed usage") { SECTION("unable to open") { // It's not elegant, but neither an empty path nor a path to a - // deleted file work for this test on Windows. + // deleted file work for this test on Windows. // // On Windows, deleting the file doesn't delete the file, and an // empty path deletes the environment variable rather than set the @@ -1100,7 +1101,8 @@ TEST_CASE("TracerConfig propagation styles") { // brevity static const auto datadog = PropagationStyle::DATADOG, - b3 = PropagationStyle::B3, none = PropagationStyle::NONE; + b3 = PropagationStyle::B3, + none = PropagationStyle::NONE; // clang-format off auto test_case = GENERATE(values({ {__LINE__, "Datadog", x, {datadog}}, From f0728d7920f03423f56b4290c880352c7e23c6ca Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sun, 7 Jan 2024 23:19:16 -0500 Subject: [PATCH 04/19] Unbreak Linux after Windows changes. --- src/datadog/extraction_util.cpp | 4 ++-- src/datadog/platform_util.cpp | 2 ++ test/test_tracer_config.cpp | 8 +++++--- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/datadog/extraction_util.cpp b/src/datadog/extraction_util.cpp index 3ef5fef5..16e1459f 100644 --- a/src/datadog/extraction_util.cpp +++ b/src/datadog/extraction_util.cpp @@ -70,7 +70,7 @@ Expected> extract_id_header(const DictReader& headers, int base) { auto found = headers.lookup(header); if (!found) { - return nullopt; + return Optional(); } auto result = parse_uint64(*found, base); if (auto* error = result.if_error()) { @@ -86,7 +86,7 @@ Expected> extract_id_header(const DictReader& headers, prefix += ' '; return error->with_prefix(prefix); } - return *result; + return Optional(*result); } Expected extract_datadog( diff --git a/src/datadog/platform_util.cpp b/src/datadog/platform_util.cpp index 13a12391..7ea303e9 100644 --- a/src/datadog/platform_util.cpp +++ b/src/datadog/platform_util.cpp @@ -52,7 +52,9 @@ void init_winsock_if_not_already() { } // namespace Optional get_hostname() { +#ifdef _MSC_VER init_winsock_if_not_already(); +#endif char buffer[256]; if (::gethostname(buffer, sizeof buffer)) { return nullopt; diff --git a/test/test_tracer_config.cpp b/test/test_tracer_config.cpp index ca86b9c7..9dbeb874 100644 --- a/test/test_tracer_config.cpp +++ b/test/test_tracer_config.cpp @@ -49,12 +49,14 @@ namespace { class EnvGuard { std::string name_; Optional former_value_; +#ifdef _MSC_VER // maximum size of an environment variable value on Windows char buffer_[32767]; +#endif public: EnvGuard(std::string name, std::string value) : name_(std::move(name)) { - const char* current = get_value(name_); + const char* current = get_value(); if (current) { former_value_ = current; } @@ -69,10 +71,10 @@ class EnvGuard { } } - const char* get_value(const std::string& name) { + const char* get_value() { #ifdef _MSC_VER const DWORD rc = - GetEnvironmentVariable(name.c_str(), buffer_, sizeof buffer_); + GetEnvironmentVariable(name_.c_str(), buffer_, sizeof buffer_); if (rc == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND) { return nullptr; } From 3caed5e0801590e6114beec462f920aec0f02a5c Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 14:33:46 -0500 Subject: [PATCH 05/19] `string_view::const_iterator` != `const char*` --- src/datadog/msgpack.h | 2 +- src/datadog/parse_util.cpp | 12 ++++++------ src/datadog/parse_util.h | 5 +++-- src/datadog/tracer_config.cpp | 4 ++-- src/datadog/w3c_propagation.cpp | 11 +++++------ 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/datadog/msgpack.h b/src/datadog/msgpack.h index 2df96f1c..52b8a919 100644 --- a/src/datadog/msgpack.h +++ b/src/datadog/msgpack.h @@ -160,7 +160,7 @@ inline void pack_integer(std::string& buffer, std::int32_t value) { } inline Expected pack_string(std::string& buffer, StringView value) { - return pack_string(buffer, value.begin(), value.size()); + return pack_string(buffer, value.data(), value.size()); } } // namespace msgpack diff --git a/src/datadog/parse_util.cpp b/src/datadog/parse_util.cpp index d7ecd1bc..1872bc8a 100644 --- a/src/datadog/parse_util.cpp +++ b/src/datadog/parse_util.cpp @@ -17,14 +17,15 @@ namespace { template Expected parse_integer(StringView input, int base, StringView kind) { Integer value; - const auto status = std::from_chars(input.begin(), input.end(), value, base); + const char* const end = input.data() + input.size(); + const auto status = std::from_chars(input.data(), end, value, base); if (status.ec == std::errc::invalid_argument) { std::string message; message += "Is not a valid integer: \""; append(message, input); message += '\"'; return Error{Error::INVALID_INTEGER, std::move(message)}; - } else if (status.ptr != input.end()) { + } else if (status.ptr != end) { std::string message; message += "Integer has trailing characters in: \""; append(message, input); @@ -47,16 +48,15 @@ StringView strip(StringView input) { const auto not_whitespace = [](unsigned char ch) { return !std::isspace(ch); }; - const char* const begin = - std::find_if(input.begin(), input.end(), not_whitespace); - const char* const end = + const auto begin = std::find_if(input.begin(), input.end(), not_whitespace); + const auto end = std::find_if(input.rbegin(), std::make_reverse_iterator(begin), not_whitespace) .base(); assert(begin <= end); - return StringView{begin, std::size_t(end - begin)}; + return range(begin, end); } Expected parse_uint64(StringView input, int base) { diff --git a/src/datadog/parse_util.h b/src/datadog/parse_util.h index 65233e87..c9d60ce3 100644 --- a/src/datadog/parse_util.h +++ b/src/datadog/parse_util.h @@ -12,8 +12,9 @@ namespace datadog { namespace tracing { // Return a `string_view` over the specified range of characters `[begin, end)`. -inline StringView range(const char* begin, const char* end) { - return StringView{begin, std::size_t(end - begin)}; +template +StringView range(CharIter1 begin, CharIter2 end) { + return StringView{&*begin, std::size_t(end - begin)}; } // Remove leading and trailing whitespace (as determined by `std::isspace`) from diff --git a/src/datadog/tracer_config.cpp b/src/datadog/tracer_config.cpp index 592a9c22..92a6d257 100644 --- a/src/datadog/tracer_config.cpp +++ b/src/datadog/tracer_config.cpp @@ -38,9 +38,9 @@ std::vector parse_list(StringView input) { return items; } - const char *const end = input.end(); + const char *const end = input.data() + input.size(); - const char *current = input.begin(); + const char *current = input.data(); const char *begin_delim; do { const char *begin_item = diff --git a/src/datadog/w3c_propagation.cpp b/src/datadog/w3c_propagation.cpp index eae5cdd5..21fac8c0 100644 --- a/src/datadog/w3c_propagation.cpp +++ b/src/datadog/w3c_propagation.cpp @@ -68,8 +68,7 @@ Optional extract_traceparent(ExtractedData& result, const auto to_string_view = [](const auto& submatch) { assert(submatch.first <= submatch.second); - return StringView{submatch.first, - std::size_t(submatch.second - submatch.first)}; + return range(submatch.first, submatch.second); }; const auto version = to_string_view(match[1]); @@ -110,8 +109,8 @@ struct PartiallyParsedTracestate { Optional parse_tracestate(StringView tracestate) { Optional result; - const char* const begin = tracestate.begin(); - const char* const end = tracestate.end(); + const char* const begin = tracestate.data(); + const char* const end = begin + tracestate.size(); const char* pair_begin = begin; while (pair_begin != end) { const char* const pair_end = std::find(pair_begin, end, ','); @@ -174,8 +173,8 @@ Optional parse_tracestate(StringView tracestate) { // - `sampling_priority` // - `additional_datadog_w3c_tracestate` void parse_datadog_tracestate(ExtractedData& result, StringView datadog_value) { - const char* const begin = datadog_value.begin(); - const char* const end = datadog_value.end(); + const char* const begin = datadog_value.data(); + const char* const end = begin + datadog_value.size(); const char* pair_begin = begin; while (pair_begin != end) { const char* const pair_end = std::find(pair_begin, end, ';'); From 9bbde2534c8d6b43b76394c254f549fca6915cb8 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 14:44:29 -0500 Subject: [PATCH 06/19] experiment with Windows CI --- .circleci/config.yml | 50 ++++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6180a568..b6e8698e 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,5 +1,8 @@ version: 2.1 +orbs: + windows: circleci/windows@5.0 + executors: docker-amd64: docker: @@ -27,6 +30,16 @@ jobs: - checkout - run: find bin/ -executable -type f -print0 | xargs -0 shellcheck + test-windows-bazel: + executor: windows/server-2022 + environment: + MAKE_JOB_COUNT: 8 + steps: + - checkout + - run: pwd + - run: ls + - run: Get-Command choco + build-bazel: parameters: toolchain: @@ -110,7 +123,7 @@ jobs: environment: TEST_LIBRARY: cpp DD_TRACE_CPP_COMMIT: << pipeline.git.revision >> - command: cd system-tests && ./build.sh -i runner && ./run.sh PARAMETRIC --log-cli-level=DEBUG + command: cd system-tests && ./build.sh -i runner && ./run.sh PARAMETRIC --log-cli-level=DEBUG - run: name: Collect artifacts command: tar -cvzf logs_cpp_parametric_dev.tar.gz -C system-tests logs_parametric @@ -124,20 +137,21 @@ workflows: jobs: - format - shellcheck - - test-cmake: - matrix: - parameters: - toolchain: ["gnu", "llvm"] - sanitize: ["on", "off"] - arch: ["amd64", "arm64"] - - build-bazel: - matrix: - parameters: - arch: ["amd64", "arm64"] - toolchain: ["gnu", "llvm"] - - coverage - - system-tests: - filters: - branches: - only: - - main + - test-windows-bazel + # - test-cmake: + # matrix: + # parameters: + # toolchain: ["gnu", "llvm"] + # sanitize: ["on", "off"] + # arch: ["amd64", "arm64"] + # - build-bazel: + # matrix: + # parameters: + # arch: ["amd64", "arm64"] + # toolchain: ["gnu", "llvm"] + # - coverage + # - system-tests: + # filters: + # branches: + # only: + # - main From c326db3ae39c8a284cbda0c8e6463cfe2bb2a9e0 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 14:52:07 -0500 Subject: [PATCH 07/19] CircleCI has choco, nice. Let's try bazelisk. --- .circleci/config.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index b6e8698e..aa4bee47 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -33,12 +33,14 @@ jobs: test-windows-bazel: executor: windows/server-2022 environment: - MAKE_JOB_COUNT: 8 + MAKE_JOB_COUNT: 4 steps: - checkout - run: pwd - run: ls - run: Get-Command choco + - run: choco install -y bazelisk + - run: bazelisk test --jobs $MAKE_JOB_COUNT dd_trace_cpp_test build-bazel: parameters: @@ -135,8 +137,8 @@ jobs: workflows: pull-request: jobs: - - format - - shellcheck + # - format + # - shellcheck - test-windows-bazel # - test-cmake: # matrix: From da204ee1a2104fb466f3d55c836263760089b0cd Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 14:59:39 -0500 Subject: [PATCH 08/19] powershell uses $env:foo, not $foo --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index aa4bee47..6b4a441b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -40,7 +40,7 @@ jobs: - run: ls - run: Get-Command choco - run: choco install -y bazelisk - - run: bazelisk test --jobs $MAKE_JOB_COUNT dd_trace_cpp_test + - run: bazelisk test --jobs $env:MAKE_JOB_COUNT dd_trace_cpp_test build-bazel: parameters: From 91b4346a5f726889595cb028c474fef3752581e7 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 15:04:14 -0500 Subject: [PATCH 09/19] remove debug logging from Windows CI config --- .circleci/config.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 6b4a441b..95f1fe9b 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -36,9 +36,6 @@ jobs: MAKE_JOB_COUNT: 4 steps: - checkout - - run: pwd - - run: ls - - run: Get-Command choco - run: choco install -y bazelisk - run: bazelisk test --jobs $env:MAKE_JOB_COUNT dd_trace_cpp_test From 39ef09760c8ded7cab285f112c69457d5f98ff36 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 15:05:02 -0500 Subject: [PATCH 10/19] windows.h goes first --- src/datadog/environment.cpp | 4 +++- test/test_tracer_config.cpp | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/datadog/environment.cpp b/src/datadog/environment.cpp index 5b59baf6..2a58b8d2 100644 --- a/src/datadog/environment.cpp +++ b/src/datadog/environment.cpp @@ -2,8 +2,10 @@ #include #ifdef _MSC_VER -#include // GetEnvironmentVariable +// clang-format off #include +#include // GetEnvironmentVariable +// clang-format on #endif #include "json.hpp" diff --git a/test/test_tracer_config.cpp b/test/test_tracer_config.cpp index 9dbeb874..0343c62f 100644 --- a/test/test_tracer_config.cpp +++ b/test/test_tracer_config.cpp @@ -24,8 +24,10 @@ #include "mocks/loggers.h" #include "test.h" #ifdef _MSC_VER -#include // SetEnvironmentVariable +// clang-format off #include +#include // SetEnvironmentVariable +// clang-format on #else #include // setenv, unsetenv #endif From 88b7d29a9e9ecd0c84f206f755613b4b49a78b0b Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 15:10:55 -0500 Subject: [PATCH 11/19] maybe it will like this better --- src/datadog/platform_util.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/datadog/platform_util.cpp b/src/datadog/platform_util.cpp index 7ea303e9..322c74c6 100644 --- a/src/datadog/platform_util.cpp +++ b/src/datadog/platform_util.cpp @@ -1,8 +1,10 @@ #include "platform_util.h" #ifdef _MSC_VER -#include +// clang-format off #include +#include +// clang-format on #else #include #include From 795ac3962957280184a1dac30d63d2e6acd04c21 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 15:28:40 -0500 Subject: [PATCH 12/19] bazel: put test headers in srcs --- .bazelrc => .bazelrc.absl | 0 BUILD.bazel | 10 +++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) rename .bazelrc => .bazelrc.absl (100%) diff --git a/.bazelrc b/.bazelrc.absl similarity index 100% rename from .bazelrc rename to .bazelrc.absl diff --git a/BUILD.bazel b/BUILD.bazel index 2b4d33be..16b8c7f1 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -121,17 +121,26 @@ cc_test( # wrapper around Catch2 "test/test.cpp", + "test/test.h", + "test/catch.hpp", # mocks "test/mocks/collectors.cpp", + "test/mocks/collectors.h", "test/mocks/dict_readers.cpp", + "test/mocks/dict_readers.h", "test/mocks/dict_writers.cpp", + "test/mocks/dict_writers.h", "test/mocks/event_schedulers.cpp", + "test/mocks/event_schedulers.h", "test/mocks/http_clients.cpp", + "test/mocks/http_clients.h", "test/mocks/loggers.cpp", + "test/mocks/loggers.h", # utilities "test/matchers.cpp", + "test/matchers.h", # test cases "test/test_cerr_logger.cpp", @@ -152,7 +161,6 @@ cc_test( "test/test_tracer.cpp", "test/test_trace_sampler.cpp", ], - includes = ["test"], deps = [ "dd_trace_cpp", ], From 9ed5186678d7115d4c93a3655db4edc1f75f7c21 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 15:28:58 -0500 Subject: [PATCH 13/19] add a non-Abseil bazel build mode --- .bazelrc | 1 + .bazelrc.std | 11 +++++++++++ .circleci/config.yml | 13 +++++++++++-- 3 files changed, 23 insertions(+), 2 deletions(-) create mode 120000 .bazelrc create mode 100644 .bazelrc.std diff --git a/.bazelrc b/.bazelrc new file mode 120000 index 00000000..4baddac0 --- /dev/null +++ b/.bazelrc @@ -0,0 +1 @@ +.bazelrc.absl \ No newline at end of file diff --git a/.bazelrc.std b/.bazelrc.std new file mode 100644 index 00000000..a39af387 --- /dev/null +++ b/.bazelrc.std @@ -0,0 +1,11 @@ +# In order to support both Unixen and Windows, different styles of compiler +# flags must be used. +# +# This .bazelrc file specifies different compiler flags for Linux/Darwin versus +# Windows. + +build --enable_platform_specific_config + +build:linux --cxxopt='-std=c++17' --cxxopt='-Wall' --cxxopt='-Wextra' --cxxopt='-pedantic' +build:macos --cxxopt='-std=c++17' --cxxopt='-Wall' --cxxopt='-Wextra' --cxxopt='-pedantic' +build:windows --cxxopt='/std:c++17' --linkopt='ws2_32.lib' diff --git a/.circleci/config.yml b/.circleci/config.yml index 95f1fe9b..43a51307 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -31,13 +31,19 @@ jobs: - run: find bin/ -executable -type f -print0 | xargs -0 shellcheck test-windows-bazel: + parameters: + # `bazelrc` is the path to the .bazelrc file to use in the build/test. + # The repository has two flavors: one that uses Abseil types (for use with + # Envoy), and one that uses std types. + bazelrc: + type: string executor: windows/server-2022 environment: MAKE_JOB_COUNT: 4 steps: - checkout - run: choco install -y bazelisk - - run: bazelisk test --jobs $env:MAKE_JOB_COUNT dd_trace_cpp_test + - run: bazelisk --bazelrc=<< parameters.bazelrc >> test --jobs $env:MAKE_JOB_COUNT dd_trace_cpp_test build-bazel: parameters: @@ -136,7 +142,10 @@ workflows: jobs: # - format # - shellcheck - - test-windows-bazel + - test-windows-bazel: + matrix: + parameters: + bazelrc: [".bazelrc.absl", ".bazelrc.std"] # - test-cmake: # matrix: # parameters: From 59dc64fe37b82a44e3a9d31f114d5f1e60fcf90d Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 15:37:02 -0500 Subject: [PATCH 14/19] add "bazelrc" dimension to Linux bazel build/test --- .circleci/config.yml | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 43a51307..79be6271 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -45,18 +45,23 @@ jobs: - run: choco install -y bazelisk - run: bazelisk --bazelrc=<< parameters.bazelrc >> test --jobs $env:MAKE_JOB_COUNT dd_trace_cpp_test - build-bazel: + test-bazel: parameters: toolchain: type: string arch: type: string + # `bazelrc` is the path to the .bazelrc file to use in the build/test. + # The repository has two flavors: one that uses Abseil types (for use with + # Envoy), and one that uses std types. + bazelrc: + type: string executor: docker-<< parameters.arch >> environment: MAKE_JOB_COUNT: 8 steps: - checkout - - run: bin/with-toolchain << parameters.toolchain >> bazelisk build --jobs $MAKE_JOB_COUNT dd_trace_cpp + - run: bin/with-toolchain << parameters.toolchain >> bazelisk --bazelrc=<< parameters.bazelrc >> test --jobs $MAKE_JOB_COUNT dd_trace_cpp_test test-cmake: parameters: @@ -152,11 +157,12 @@ workflows: # toolchain: ["gnu", "llvm"] # sanitize: ["on", "off"] # arch: ["amd64", "arm64"] - # - build-bazel: + # - test-bazel: # matrix: # parameters: # arch: ["amd64", "arm64"] # toolchain: ["gnu", "llvm"] + # bazelrc: [".bazelrc.absl", ".bazelrc.std"] # - coverage # - system-tests: # filters: From f00680233d5c39c87ff19cf5512d453a50939f15 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 15:37:12 -0500 Subject: [PATCH 15/19] mention the difference between the two bazelrc files --- .bazelrc.absl | 4 ++++ .bazelrc.std | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/.bazelrc.absl b/.bazelrc.absl index ca2fef1e..3fd91881 100644 --- a/.bazelrc.absl +++ b/.bazelrc.absl @@ -3,6 +3,10 @@ # # This .bazelrc file specifies different compiler flags for Linux/Darwin versus # Windows. +# +# This bazelrc defines the `DD_USE_ABSEIL_FOR_ENVOY` preprocessor macro, and so +# the resulting library will use `absl::string_view` and `absl::optional` +# instead of their standard (`std`) equivalents. build --enable_platform_specific_config diff --git a/.bazelrc.std b/.bazelrc.std index a39af387..802f014d 100644 --- a/.bazelrc.std +++ b/.bazelrc.std @@ -3,6 +3,10 @@ # # This .bazelrc file specifies different compiler flags for Linux/Darwin versus # Windows. +# +# This bazelrc does _not_ define the `DD_USE_ABSEIL_FOR_ENVOY` preprocessor +# macro, and so the resulting library will use `std::string_view` and +# `std::optional` instead of their Abseil equivalents. build --enable_platform_specific_config From 3faf6f3569beedbe5e8e88b8eee1f9ddf5cdb0b8 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Sat, 13 Jan 2024 15:38:02 -0500 Subject: [PATCH 16/19] uncomment the other CI jobs --- .circleci/config.yml | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 79be6271..37a2893e 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -145,27 +145,27 @@ jobs: workflows: pull-request: jobs: - # - format - # - shellcheck + - format + - shellcheck - test-windows-bazel: matrix: parameters: bazelrc: [".bazelrc.absl", ".bazelrc.std"] - # - test-cmake: - # matrix: - # parameters: - # toolchain: ["gnu", "llvm"] - # sanitize: ["on", "off"] - # arch: ["amd64", "arm64"] - # - test-bazel: - # matrix: - # parameters: - # arch: ["amd64", "arm64"] - # toolchain: ["gnu", "llvm"] - # bazelrc: [".bazelrc.absl", ".bazelrc.std"] - # - coverage - # - system-tests: - # filters: - # branches: - # only: - # - main + - test-cmake: + matrix: + parameters: + toolchain: ["gnu", "llvm"] + sanitize: ["on", "off"] + arch: ["amd64", "arm64"] + - test-bazel: + matrix: + parameters: + arch: ["amd64", "arm64"] + toolchain: ["gnu", "llvm"] + bazelrc: [".bazelrc.absl", ".bazelrc.std"] + - coverage + - system-tests: + filters: + branches: + only: + - main From 56fe5cef00f0a7e66bf20e91199ebd62032d2786 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 15 Mar 2024 15:09:51 -0400 Subject: [PATCH 17/19] use std::getenv on Windows in environment.cpp --- src/datadog/environment.cpp | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/datadog/environment.cpp b/src/datadog/environment.cpp index 2a58b8d2..825de301 100644 --- a/src/datadog/environment.cpp +++ b/src/datadog/environment.cpp @@ -1,12 +1,6 @@ #include "environment.h" #include -#ifdef _MSC_VER -// clang-format off -#include -#include // GetEnvironmentVariable -// clang-format on -#endif #include "json.hpp" @@ -16,20 +10,10 @@ namespace environment { namespace { Optional get_env(const char *name) { -#ifdef _MSC_VER - // maximum size of an environment variable value on Windows - char buffer[32767]; - const DWORD rc = GetEnvironmentVariable(name, buffer, sizeof buffer); - if (rc == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND) { - return nullopt; - } - return std::string(buffer, rc); // `rc` is the length on success -#else if (const char *value = std::getenv(name)) { return value; } return nullopt; -#endif } } // namespace From fd07f8660a79281dd850460a060676a787ef3e53 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 15 Mar 2024 15:22:04 -0400 Subject: [PATCH 18/19] use _putenv_s on Windows in test_tracer_config.cpp --- test/test_tracer_config.cpp | 30 ++++-------------------------- 1 file changed, 4 insertions(+), 26 deletions(-) diff --git a/test/test_tracer_config.cpp b/test/test_tracer_config.cpp index 0343c62f..210fdf20 100644 --- a/test/test_tracer_config.cpp +++ b/test/test_tracer_config.cpp @@ -4,6 +4,7 @@ #include #include #include +#include // setenv (windows/unix), unsetenv (unix), _putenv_s (windows) #include #include @@ -23,14 +24,6 @@ #include "mocks/http_clients.h" #include "mocks/loggers.h" #include "test.h" -#ifdef _MSC_VER -// clang-format off -#include -#include // SetEnvironmentVariable -// clang-format on -#else -#include // setenv, unsetenv -#endif namespace datadog { namespace tracing { @@ -51,10 +44,6 @@ namespace { class EnvGuard { std::string name_; Optional former_value_; -#ifdef _MSC_VER - // maximum size of an environment variable value on Windows - char buffer_[32767]; -#endif public: EnvGuard(std::string name, std::string value) : name_(std::move(name)) { @@ -73,22 +62,11 @@ class EnvGuard { } } - const char* get_value() { -#ifdef _MSC_VER - const DWORD rc = - GetEnvironmentVariable(name_.c_str(), buffer_, sizeof buffer_); - if (rc == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND) { - return nullptr; - } - return buffer_; -#else - return std::getenv(name_.c_str()); -#endif - } + const char* get_value() { return std::getenv(name_.c_str()); } void set_value(const std::string& value) { #ifdef _MSC_VER - REQUIRE(::SetEnvironmentVariable(name_.c_str(), value.c_str())); + ::_putenv_s(name_.c_str(), value.c_str()); #else const bool overwrite = true; ::setenv(name_.c_str(), value.c_str(), overwrite); @@ -97,7 +75,7 @@ class EnvGuard { void unset() { #ifdef _MSC_VER - REQUIRE(::SetEnvironmentVariable(name_.c_str(), NULL)); + ::_putenv_s(name_.c_str(), ""); #else ::unsetenv(name_.c_str()); #endif From 11d3170392b5de38e31666b8e4766feabdb3aea7 Mon Sep 17 00:00:00 2001 From: David Goffredo Date: Fri, 15 Mar 2024 16:56:44 -0400 Subject: [PATCH 19/19] somebody missed a spot --- bin/bazel-build | 2 ++ bin/check | 3 ++- src/datadog/trace_sampler_config.cpp | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/bin/bazel-build b/bin/bazel-build index 4c295c0d..de6be237 100755 --- a/bin/bazel-build +++ b/bin/bazel-build @@ -5,3 +5,5 @@ set -e cd "$(dirname "$0")"/.. bazelisk build dd_trace_cpp + +bazelisk "$@" test dd_trace_cpp_test diff --git a/bin/check b/bin/check index 79c4f5b2..c339bbc2 100755 --- a/bin/check +++ b/bin/check @@ -10,5 +10,6 @@ cd "$(dirname "$0")"/.. bin/format --dry-run -Werror bin/test -bin/bazel-build +bin/bazel-build --bazelrc=.bazelrc.absl +bin/bazel-build --bazelrc=.bazelrc.std find bin/ -executable -type f -print0 | xargs -0 shellcheck diff --git a/src/datadog/trace_sampler_config.cpp b/src/datadog/trace_sampler_config.cpp index ac4afcd6..4a2cc229 100644 --- a/src/datadog/trace_sampler_config.cpp +++ b/src/datadog/trace_sampler_config.cpp @@ -226,7 +226,7 @@ Expected finalize_config( message += "Trace sampling max_per_second must be greater than zero, but the " "following value was given: "; - message += std::to_string(*config.max_per_second); + message += std::to_string(max_per_second); return Error{Error::MAX_PER_SECOND_OUT_OF_RANGE, std::move(message)}; } result.max_per_second = max_per_second;