diff --git a/changelogs/current.yaml b/changelogs/current.yaml index ada0b0abf0cb1..66cbefcd83471 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -123,6 +123,9 @@ bug_fixes: change: | Fixed a bug where a histogram bucket counts were wrong. Additionally, the number of buckets is fixed and is now one element larger than the explicit bounds elements, as required by the specification. +- area: tracing + change: | + Fixed a bug where child spans produced by the Datadog tracer would have an incorrect operation name. - area: DNS change: | Fixed a race condition that when multiple requests with the same authority header are sent to Envoy, sometimes some requests diff --git a/source/extensions/tracers/datadog/span.cc b/source/extensions/tracers/datadog/span.cc index d027419152187..bb15fd9fd1a9b 100644 --- a/source/extensions/tracers/datadog/span.cc +++ b/source/extensions/tracers/datadog/span.cc @@ -90,8 +90,13 @@ Tracing::SpanPtr Span::spawnChild(const Tracing::Config&, const std::string& nam // The OpenTracing implementation ignored the `Tracing::Config` argument, // so we will as well. + // The `name` parameter to this function more closely matches Datadog's + // concept of "resource name." Datadog's "span name," or "operation name," + // instead describes the category of operation being performed, which here + // we hard-code. datadog::tracing::SpanConfig config; - config.name = name; + config.name = "envoy.proxy"; + config.resource = name; config.start = estimateTime(start_time); return std::make_unique(span_->create_child(config)); diff --git a/test/extensions/tracers/datadog/BUILD b/test/extensions/tracers/datadog/BUILD index d292aa9c0f393..4eba5b353436c 100644 --- a/test/extensions/tracers/datadog/BUILD +++ b/test/extensions/tracers/datadog/BUILD @@ -19,6 +19,7 @@ envoy_extension_cc_test( "dict_util_test.cc", "event_scheduler_test.cc", "logger_test.cc", + "naming_test.cc", "span_test.cc", "time_util_test.cc", "tracer_stats_test.cc", diff --git a/test/extensions/tracers/datadog/naming_test.cc b/test/extensions/tracers/datadog/naming_test.cc new file mode 100644 index 0000000000000..0d45cde32b1ac --- /dev/null +++ b/test/extensions/tracers/datadog/naming_test.cc @@ -0,0 +1,223 @@ +/** + * The tests in this file aren't specific to a class, but instead test all + * behavior related to spans' "operation name" (a.k.a. "span name"), + * "resource name," and "service name." + * + * Datadog's model of a span is different from Envoy's. Each Datadog span, + * in addition to having a "service name" and an "operation name," also has a + * "resource name." The operation name indicates the _kind_ of operation + * that is being performed by the service, whereas the resource name contains + * more specifics about what is being operated upon, or about what is doing the + * operating. Envoy has no notion of "resource name," and instead uses + * operation name and tags for this purpose. + * + * When Envoy's tracing interface indicates an operation name, the Datadog + * tracer translates it into a resource name instead. The actual Datadog + * operation name is always hard-coded to the value "envoy.proxy". + * + * Finally, each span's "service name" is derived either from the tracer's + * configuration or a hard-coded default, which is "envoy". + * + * The tests in this file verify all of this behavior for a variety of + * scenarios where spans are created or modified. + */ + +#include "source/extensions/tracers/datadog/config.h" +#include "source/extensions/tracers/datadog/span.h" +#include "source/extensions/tracers/datadog/tracer.h" + +#include "test/mocks/stream_info/mocks.h" +#include "test/mocks/thread_local/mocks.h" +#include "test/mocks/tracing/mocks.h" +#include "test/mocks/upstream/cluster_manager.h" +#include "test/test_common/simulated_time_system.h" +#include "test/test_common/utility.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Extensions { +namespace Tracers { +namespace Datadog { +namespace { + +template Config makeConfig(const std::string& yaml) { + Config result; + TestUtility::loadFromYaml(yaml, result); + return result; +} + +class DatadogTracerNamingTest : public testing::Test { +public: + DatadogTracerNamingTest(); + +protected: + /** + * Verify that a tracer configured using the specified \p config_yaml + * produces spans and child spans having the specified + * \p expected_service_name. + * @param config_yaml YAML representation of a + * \c type.googleapis.com/envoy.config.trace.v3.DatadogConfig + * @param expected_service_name service name to expect each span to have + */ + void serviceNameTest(const std::string& config_yaml, const std::string& expected_service_name); + + /** + * Assign through the specified \p result a pointer to the underlying + * \c datadog::tracing::Span to which the specified \p span refers. + * If \p span does not refer to a Datadog span, then this function triggers a + * fatal test assertion. + * An output parameter is used because the \c ASSERT_* macros require that + * the enclosing function have \c void return type. + * @param result pointer to the output value to overwrite + * @param span pointer to an Envoy span that refers to a Datadog span + */ + static void asDatadogSpan(const datadog::tracing::Span** result, const Tracing::SpanPtr& span); + + NiceMock cluster_manager_; + Stats::TestUtil::TestStore store_; + NiceMock thread_local_slot_allocator_; + Event::SimulatedTimeSystem time_; + NiceMock stream_info_; +}; + +DatadogTracerNamingTest::DatadogTracerNamingTest() { + cluster_manager_.initializeClusters({"fake_cluster"}, {}); + cluster_manager_.thread_local_cluster_.cluster_.info_->name_ = "fake_cluster"; + cluster_manager_.initializeThreadLocalClusters({"fake_cluster"}); +} + +void DatadogTracerNamingTest::asDatadogSpan(const datadog::tracing::Span** result, + const Tracing::SpanPtr& span) { + ASSERT_TRUE(span); + const auto as_dd_span_wrapper = dynamic_cast(span.get()); + ASSERT_NE(nullptr, as_dd_span_wrapper); + + const datadog::tracing::Optional& maybe_dd_span = + as_dd_span_wrapper->impl(); + ASSERT_TRUE(maybe_dd_span); + *result = &*maybe_dd_span; +} + +void DatadogTracerNamingTest::serviceNameTest(const std::string& config_yaml, + const std::string& expected_service_name) { + auto config_proto = makeConfig(config_yaml); + + Tracer tracer{config_proto.collector_cluster(), + config_proto.collector_hostname(), + DatadogTracerFactory::makeConfig(config_proto), + cluster_manager_, + *store_.rootScope(), + thread_local_slot_allocator_}; + + // Any values will do for the sake of this test. What we care about is the + // `expected_service_name`. + Tracing::Decision decision; + decision.reason = Tracing::Reason::Sampling; + decision.traced = true; + const std::string operation_name = "some.operation.name"; + Tracing::TestTraceContextImpl context{}; + + const Tracing::SpanPtr span = + tracer.startSpan(Tracing::MockConfig{}, context, stream_info_, operation_name, decision); + const datadog::tracing::Span* dd_span; + asDatadogSpan(&dd_span, span); + + EXPECT_EQ(expected_service_name, dd_span->service_name()); + + const auto child_start = time_.timeSystem().systemTime(); + const std::string child_operation_name = "some.other.operation.name"; + const Tracing::SpanPtr child = + span->spawnChild(Tracing::MockConfig{}, child_operation_name, child_start); + const datadog::tracing::Span* dd_child; + asDatadogSpan(&dd_child, child); + + EXPECT_EQ(expected_service_name, dd_child->service_name()); +} + +TEST_F(DatadogTracerNamingTest, ServiceNameConfigured) { + // If you specify a `service_name` in the tracer configuration, then spans + // created will have that service name. + serviceNameTest(R"EOF( + collector_cluster: fake_cluster + service_name: mr_bigglesworth + )EOF", + "mr_bigglesworth"); +} + +TEST_F(DatadogTracerNamingTest, ServiceNameDefault) { + // If you don't specify a `service_name` in the tracer configuration, then + // spans created will have the default service name, which is "envoy". + serviceNameTest(R"EOF( + collector_cluster: fake_cluster + )EOF", + "envoy"); +} + +TEST_F(DatadogTracerNamingTest, OperationNameAndResourceName) { + // Concerns: + // + // - The span returned by `Tracer::startSpan` has as its resource name the + // operation name passed to `Tracer::startSpan`, and has as its operation + // name "envoy.proxy". + // - The span returned by `Span::spawnChild` has as its resource name the + // operation name passed to `Tracer::spawnChild`, and has as its operation + // name "envoy.proxy". + // - `Span::setOperation` sets the resource name of the span, but does not + // change the operation name. + + auto config_proto = makeConfig(R"EOF( + collector_cluster: fake_cluster + )EOF"); + + Tracer tracer{config_proto.collector_cluster(), + config_proto.collector_hostname(), + DatadogTracerFactory::makeConfig(config_proto), + cluster_manager_, + *store_.rootScope(), + thread_local_slot_allocator_}; + + // Any values will do for the sake of this test. What we care about are the + // operation names and the resource names. + Tracing::Decision decision; + decision.reason = Tracing::Reason::Sampling; + decision.traced = true; + Tracing::TestTraceContextImpl context{}; + + const std::string operation_name = "some.operation.name"; + const Tracing::SpanPtr span = + tracer.startSpan(Tracing::MockConfig{}, context, stream_info_, operation_name, decision); + const datadog::tracing::Span* dd_span; + asDatadogSpan(&dd_span, span); + + EXPECT_EQ("envoy.proxy", dd_span->name()); + EXPECT_EQ(operation_name, dd_span->resource_name()); + + const std::string new_operation_name = "some.new.operation.name"; + span->setOperation(new_operation_name); + + EXPECT_EQ("envoy.proxy", dd_span->name()); + EXPECT_EQ(new_operation_name, dd_span->resource_name()); + + const auto child_start = time_.timeSystem().systemTime(); + const std::string child_operation_name = "some.child.operation.name"; + const Tracing::SpanPtr child = + span->spawnChild(Tracing::MockConfig{}, child_operation_name, child_start); + const datadog::tracing::Span* dd_child; + asDatadogSpan(&dd_child, child); + + EXPECT_EQ("envoy.proxy", dd_child->name()); + EXPECT_EQ(child_operation_name, dd_child->resource_name()); + + const std::string child_new_operation_name = "some.child.new.operation.name"; + child->setOperation(child_new_operation_name); + + EXPECT_EQ("envoy.proxy", dd_child->name()); + EXPECT_EQ(child_new_operation_name, dd_child->resource_name()); +} + +} // namespace +} // namespace Datadog +} // namespace Tracers +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/tracers/datadog/span_test.cc b/test/extensions/tracers/datadog/span_test.cc index 20ea40e4c26f5..436685b36f3a0 100644 --- a/test/extensions/tracers/datadog/span_test.cc +++ b/test/extensions/tracers/datadog/span_test.cc @@ -206,7 +206,11 @@ TEST_F(DatadogTracerSpanTest, SpawnChild) { EXPECT_NE(nullptr, child_ptr); const datadog::tracing::SpanData& child = *child_ptr; EXPECT_EQ(estimateTime(child_start).wall, child.start.wall); - EXPECT_EQ("child", child.name); + // Setting the operation name actually sets the resource name, because + // Envoy's notion of operation name more closely matches Datadog's notion of + // resource name. The actual operation name is hard-coded as "envoy.proxy". + EXPECT_EQ("child", child.resource); + EXPECT_EQ("envoy.proxy", child.name); EXPECT_EQ(id_, child.trace_id); EXPECT_EQ(id_, child.span_id); EXPECT_EQ(id_, child.parent_id);