From a1edacdc8937a0702d7a47a2bc75ef01456ad3bf Mon Sep 17 00:00:00 2001 From: Ashish Banerjee Date: Thu, 19 Sep 2024 21:21:02 +0000 Subject: [PATCH 1/9] Add setOperation patch --- .../foreign_cc/0005-otel-set-operation.patch | 139 ++++++++++++++++++ bazel/repositories.bzl | 1 + 2 files changed, 140 insertions(+) create mode 100644 bazel/foreign_cc/0005-otel-set-operation.patch diff --git a/bazel/foreign_cc/0005-otel-set-operation.patch b/bazel/foreign_cc/0005-otel-set-operation.patch new file mode 100644 index 00000000..e21fa961 --- /dev/null +++ b/bazel/foreign_cc/0005-otel-set-operation.patch @@ -0,0 +1,139 @@ +diff --git a/source/extensions/tracers/opentelemetry/tracer.cc b/source/extensions/tracers/opentelemetry/tracer.cc +index 544c37ef96..df64692a37 100644 +--- source/extensions/tracers/opentelemetry/tracer.cc ++++ source/extensions/tracers/opentelemetry/tracer.cc +@@ -83,6 +83,8 @@ void Span::finishSpan() { + } + } + ++void Span::setOperation(absl::string_view operation) { span_.set_name(operation); }; ++ + void Span::injectContext(Tracing::TraceContext& trace_context, + const Upstream::HostDescriptionConstSharedPtr&) { + std::string trace_id_hex = absl::BytesToHexString(span_.trace_id()); +diff --git a/source/extensions/tracers/opentelemetry/tracer.h b/source/extensions/tracers/opentelemetry/tracer.h +index b63e54d44a69..2c9bb216b411 100644 +--- source/extensions/tracers/opentelemetry/tracer.h ++++ source/extensions/tracers/opentelemetry/tracer.h +@@ -85,7 +85,7 @@ class Span : Logger::Loggable, public Tracing::Span { + Tracer& parent_tracer, OTelSpanKind span_kind); + + // Tracing::Span functions +- void setOperation(absl::string_view /*operation*/) override{}; ++ void setOperation(absl::string_view /*operation*/) override; + void setTag(absl::string_view /*name*/, absl::string_view /*value*/) override; + void log(SystemTime /*timestamp*/, const std::string& /*event*/) override{}; + void finishSpan() override; +@@ -121,6 +121,11 @@ class Span : Logger::Loggable, public Tracing::Span { + + OTelSpanKind spankind() const { return span_.kind(); } + ++ /** ++ * @return the operation name set on the span ++ */ ++ std::string name() const { return span_.name(); } ++ + /** + * Sets the span's id. + */ +diff --git a/test/extensions/tracers/opentelemetry/BUILD b/test/extensions/tracers/opentelemetry/BUILD +index f96040354c42..c336cc9119d3 100644 +--- test/extensions/tracers/opentelemetry/BUILD ++++ test/extensions/tracers/opentelemetry/BUILD +@@ -101,3 +101,19 @@ envoy_extension_cc_test( + "//test/test_common:utility_lib", + ], + ) ++ ++envoy_extension_cc_test( ++ name = "operation_name_test", ++ srcs = ["operation_name_test.cc"], ++ extension_names = ["envoy.tracers.opentelemetry"], ++ deps = [ ++ "//source/extensions/tracers/opentelemetry:config", ++ "//source/extensions/tracers/opentelemetry:opentelemetry_tracer_lib", ++ "//test/mocks/server:tracer_factory_context_mocks", ++ "//test/mocks/stream_info:stream_info_mocks", ++ "//test/mocks/thread_local:thread_local_mocks", ++ "//test/mocks/tracing:tracing_mocks", ++ "//test/mocks/upstream:cluster_manager_mocks", ++ "//test/test_common:utility_lib", ++ ], ++) +diff --git a/test/extensions/tracers/opentelemetry/operation_name_test.cc b/test/extensions/tracers/opentelemetry/operation_name_test.cc +new file mode 100644 +index 000000000000..0d96159e4053 +--- /dev/null ++++ test/extensions/tracers/opentelemetry/operation_name_test.cc +@@ -0,0 +1,71 @@ ++#include "source/extensions/tracers/opentelemetry/config.h" ++#include "source/extensions/tracers/opentelemetry/opentelemetry_tracer_impl.h" ++#include "source/extensions/tracers/opentelemetry/tracer.h" ++ ++#include "test/mocks/server/tracer_factory_context.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/utility.h" ++ ++#include "gtest/gtest.h" ++ ++namespace Envoy { ++namespace Extensions { ++namespace Tracers { ++namespace OpenTelemetry { ++ ++class OpenTelemetryTracerOperationNameTest : public testing::Test { ++public: ++ OpenTelemetryTracerOperationNameTest(); ++ ++protected: ++ NiceMock config; ++ NiceMock stream_info; ++ Tracing::TestTraceContextImpl trace_context{}; ++ NiceMock context; ++ NiceMock cluster_manager_; ++}; ++ ++OpenTelemetryTracerOperationNameTest::OpenTelemetryTracerOperationNameTest() { ++ cluster_manager_.initializeClusters({"fake-cluster"}, {}); ++ cluster_manager_.thread_local_cluster_.cluster_.info_->name_ = "fake-cluster"; ++ cluster_manager_.initializeThreadLocalClusters({"fake-cluster"}); ++} ++ ++TEST_F(OpenTelemetryTracerOperationNameTest, OperationName) { ++ // Checks: ++ // ++ // - The span returned by `Tracer::startSpan` has as its name the ++ // operation name passed to `Tracer::startSpan` ++ // - `Span::setOperation` sets the name of the span ++ ++ const std::string yaml_string = R"EOF( ++ grpc_service: ++ envoy_grpc: ++ cluster_name: fake-cluster ++ timeout: 0.250s ++ service_name: test-service-name ++ )EOF"; ++ ++ envoy::config::trace::v3::OpenTelemetryConfig opentelemetry_config; ++ TestUtility::loadFromYaml(yaml_string, opentelemetry_config); ++ ++ auto driver = std::make_unique(opentelemetry_config, context); ++ ++ const std::string operation_name = "initial_operation_name"; ++ Tracing::SpanPtr tracing_span = driver->startSpan( ++ config, trace_context, stream_info, operation_name, {Tracing::Reason::Sampling, true}); ++ ++ EXPECT_EQ(dynamic_cast(tracing_span.get())->name(), operation_name); ++ ++ const std::string new_operation_name = "the_new_operation_name"; ++ tracing_span->setOperation(new_operation_name); ++ EXPECT_EQ(dynamic_cast(tracing_span.get())->name(), new_operation_name); ++} ++ ++} // namespace OpenTelemetry ++} // namespace Tracers ++} // namespace Extensions ++} // namespace Envoy diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index f5def418..71325e0b 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -71,6 +71,7 @@ def envoy_gloo_dependencies(): "@envoy_gloo//bazel/foreign_cc:0002-ratelimit-filter-state-hits-addend.patch", "@envoy_gloo//bazel/foreign_cc:0003-deallocate-slots-on-worker-threads.patch", # https://github.com/envoyproxy/envoy/pull/33395 "@envoy_gloo//bazel/foreign_cc:0004-local-rate-limit-bucket-backport.patch", + "@envoy_gloo//bazel/foreign_cc:0005-otel-set-operation.patch", ]) _repository_impl("json", build_file = "@envoy_gloo//bazel/external:json.BUILD") _repository_impl("inja", build_file = "@envoy_gloo//bazel/external:inja.BUILD") From 74fb9ce7d7a64618d26ae3b9c811067b603967b3 Mon Sep 17 00:00:00 2001 From: Ashish Banerjee Date: Thu, 26 Sep 2024 11:50:20 -0400 Subject: [PATCH 2/9] Add changelog --- .../v1.30.6-patch2/opentelemetry-set-operation-patch.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/v1.30.6-patch2/opentelemetry-set-operation-patch.yaml diff --git a/changelog/v1.30.6-patch2/opentelemetry-set-operation-patch.yaml b/changelog/v1.30.6-patch2/opentelemetry-set-operation-patch.yaml new file mode 100644 index 00000000..184b8b1d --- /dev/null +++ b/changelog/v1.30.6-patch2/opentelemetry-set-operation-patch.yaml @@ -0,0 +1,6 @@ +changelog: +- type: FIX + issueLink: https://github.com/solo-io/gloo/issues/9848 + resolvesIssue: false + description: >- + Backport a patch that allows setting the span name for OpenTelemetry traces. From dbeabf3e8604b2f6fb0ece57b54326ee92c5e9b5 Mon Sep 17 00:00:00 2001 From: Ashish Banerjee Date: Fri, 27 Sep 2024 20:58:55 +0000 Subject: [PATCH 3/9] Transformation initial implementation Should work but does not seem to at the moment --- .../transformation/v2/transformation_filter.proto | 10 ++++++++++ .../http/transformation/inja_transformer.cc | 15 ++++++++++++++- .../http/transformation/inja_transformer.h | 1 + 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/filter/http/transformation/v2/transformation_filter.proto b/api/envoy/config/filter/http/transformation/v2/transformation_filter.proto index 067e2bad..04cb495c 100644 --- a/api/envoy/config/filter/http/transformation/v2/transformation_filter.proto +++ b/api/envoy/config/filter/http/transformation/v2/transformation_filter.proto @@ -320,6 +320,16 @@ message TransformationTemplate { // for more information. string string_delimiter = 14; + message SpanTransformer { + // A template that sets the span name + InjaTemplate name = 1; + + // TODO if we want to set attributes as well, add fields to modify them here. + } + + // Use this field to modify the span of the trace. + SpanTransformer span_transformer = 15; + } // Defines an [Inja template](https://github.com/pantor/inja) that will be diff --git a/source/extensions/filters/http/transformation/inja_transformer.cc b/source/extensions/filters/http/transformation/inja_transformer.cc index b5c0947e..ed09a0e1 100644 --- a/source/extensions/filters/http/transformation/inja_transformer.cc +++ b/source/extensions/filters/http/transformation/inja_transformer.cc @@ -833,6 +833,14 @@ InjaTransformer::InjaTransformer(const TransformationTemplate &transformation, } } + if (transformation.has_span_transformer() && transformation.span_transformer().has_name()) { + try { + span_name_template_.emplace(instance_->parse(transformation.span_transformer().name().text())); + } catch (const std::exception &e) { + throw EnvoyException( + fmt::format("Failed to parse span name template {}", e.what())); + } + } } InjaTransformer::~InjaTransformer() {} @@ -980,7 +988,6 @@ void InjaTransformer::transform(Http::RequestOrResponseHeaderMap &header_map, typed_tls_data.endpoint_metadata_ = endpoint_metadata; typed_tls_data.metadata_string_delimiter_ = metadata_string_delimiter_; - // Body transform: absl::optional maybe_body; @@ -1063,6 +1070,12 @@ void InjaTransformer::transform(Http::RequestOrResponseHeaderMap &header_map, } } + // Span transform: + if (span_name_template_.has_value()) { + std::string output = instance_->render(span_name_template_.value()); + callbacks.activeSpan().setOperation(output); + } + // replace body. we do it here so that headers and dynamic metadata have the // original body. if (maybe_body.has_value()) { diff --git a/source/extensions/filters/http/transformation/inja_transformer.h b/source/extensions/filters/http/transformation/inja_transformer.h index 20e4a89d..e5234391 100644 --- a/source/extensions/filters/http/transformation/inja_transformer.h +++ b/source/extensions/filters/http/transformation/inja_transformer.h @@ -155,6 +155,7 @@ class InjaTransformer : public Transformer, Logger::Loggable bool escape_characters_{}; absl::optional body_template_; + absl::optional span_name_template_; bool merged_extractors_to_body_{}; // merged_templates_ is a vector of tuples with the following fields: // 1. The json path to merge the template into From 258b74fa5a811135979bccc7b02ae50548373cf6 Mon Sep 17 00:00:00 2001 From: Ashish Banerjee Date: Sat, 28 Sep 2024 02:18:34 +0000 Subject: [PATCH 4/9] Implement unit test for setting span name --- .../filters/http/transformation/BUILD | 1 + .../transformation/inja_transformer_test.cc | 24 +++++++++++++++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/transformation/BUILD b/test/extensions/filters/http/transformation/BUILD index 344cc91a..27e810ea 100644 --- a/test/extensions/filters/http/transformation/BUILD +++ b/test/extensions/filters/http/transformation/BUILD @@ -25,6 +25,7 @@ envoy_gloo_cc_test( "@envoy//test/mocks/http:http_mocks", "@envoy//test/mocks/server:server_mocks", "@envoy//test/mocks/upstream:upstream_mocks", + "@envoy//test/mocks/tracing:tracing_mocks", ], ) diff --git a/test/extensions/filters/http/transformation/inja_transformer_test.cc b/test/extensions/filters/http/transformation/inja_transformer_test.cc index c3d5b6b9..c737fde7 100644 --- a/test/extensions/filters/http/transformation/inja_transformer_test.cc +++ b/test/extensions/filters/http/transformation/inja_transformer_test.cc @@ -7,6 +7,7 @@ #include "test/mocks/http/mocks.h" #include "test/mocks/server/mocks.h" #include "test/mocks/thread_local/mocks.h" +#include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/environment.h" @@ -1237,6 +1238,25 @@ TEST_F(InjaTransformerTest, ParseFromClusterMetadata) { EXPECT_EQ(body.toString(), "val"); } +TEST_F(InjaTransformerTest, SetSpanName) { + using ::envoy::api::v2::filter::http::TransformationTemplate_SpanTransformer; + std::string test_span_name = "TEST_SPAN_NAME"; + Http::TestRequestHeaderMapImpl headers{ + {":method", "GET"}, + {":path", "/"}, + }; + TransformationTemplate transformation; + transformation.mutable_span_transformer()->mutable_name()->set_text(test_span_name); + + NiceMock callbacks; + InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_); + Buffer::OwnedImpl body(""); + std::unique_ptr mock_span = std::make_unique(); + EXPECT_CALL(callbacks, activeSpan).WillOnce(ReturnRef(*mock_span)); + EXPECT_CALL(*mock_span, setOperation(test_span_name)).Times(1); + transformer.transform(headers, &headers, body, callbacks); +} + const std::string NESTED_KEY = R"EOF( { @@ -1246,7 +1266,7 @@ const std::string NESTED_KEY = } )EOF"; -TEST_F(InjaTransformerTest, ParseFromDynamicmeMetadata) { +TEST_F(InjaTransformerTest, ParseFromDynamicMetadata) { Http::TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/foo"}}; TransformationTemplate transformation; transformation.mutable_body()->set_text("{{dynamic_metadata(\"key:value\")}}"); @@ -1280,7 +1300,7 @@ const std::string NESTED_LIST = } )EOF"; -TEST_F(InjaTransformerTest, ParseFromDynamicmeMetadataList) { +TEST_F(InjaTransformerTest, ParseFromDynamicMetadataList) { Http::TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/foo"}}; TransformationTemplate transformation; transformation.mutable_body()->set_text("{{dynamic_metadata(\"key:value\")}}"); From ddbc2289a305684e9f4abb9ca74c3da07c17e2b3 Mon Sep 17 00:00:00 2001 From: Ashish Banerjee Date: Fri, 27 Sep 2024 22:27:08 -0400 Subject: [PATCH 5/9] Update changelog --- .../opentelemetry-set-operation-patch.yaml | 5 +++++ 1 file changed, 5 insertions(+) rename changelog/{v1.30.6-patch2 => v1.30.6-patch3}/opentelemetry-set-operation-patch.yaml (50%) diff --git a/changelog/v1.30.6-patch2/opentelemetry-set-operation-patch.yaml b/changelog/v1.30.6-patch3/opentelemetry-set-operation-patch.yaml similarity index 50% rename from changelog/v1.30.6-patch2/opentelemetry-set-operation-patch.yaml rename to changelog/v1.30.6-patch3/opentelemetry-set-operation-patch.yaml index 184b8b1d..e214c664 100644 --- a/changelog/v1.30.6-patch2/opentelemetry-set-operation-patch.yaml +++ b/changelog/v1.30.6-patch3/opentelemetry-set-operation-patch.yaml @@ -1,4 +1,9 @@ changelog: +- type: NEW_FEATURE + issueLink: https://github.com/solo-io/gloo/issues/9848 + resolvesIssue: false + description: >- + Update the transformation filter to allow setting the tracer's span name. - type: FIX issueLink: https://github.com/solo-io/gloo/issues/9848 resolvesIssue: false From b1714d0baca09e13c51f1bd08e4a383c2d55c73d Mon Sep 17 00:00:00 2001 From: Ashish Banerjee Date: Fri, 27 Sep 2024 22:30:47 -0400 Subject: [PATCH 6/9] Remove extra entry from changelog --- .../v1.30.6-patch3/opentelemetry-set-operation-patch.yaml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/changelog/v1.30.6-patch3/opentelemetry-set-operation-patch.yaml b/changelog/v1.30.6-patch3/opentelemetry-set-operation-patch.yaml index e214c664..20328699 100644 --- a/changelog/v1.30.6-patch3/opentelemetry-set-operation-patch.yaml +++ b/changelog/v1.30.6-patch3/opentelemetry-set-operation-patch.yaml @@ -4,8 +4,3 @@ changelog: resolvesIssue: false description: >- Update the transformation filter to allow setting the tracer's span name. -- type: FIX - issueLink: https://github.com/solo-io/gloo/issues/9848 - resolvesIssue: false - description: >- - Backport a patch that allows setting the span name for OpenTelemetry traces. From 690f9085734338e32906e7233dcce9e1d0331bb6 Mon Sep 17 00:00:00 2001 From: Ashish Banerjee Date: Mon, 30 Sep 2024 20:26:07 +0000 Subject: [PATCH 7/9] Do not override route.decorator.operation --- .../http/transformation/inja_transformer.cc | 11 +++- .../transformation/inja_transformer_test.cc | 61 ++++++++++++++++--- 2 files changed, 61 insertions(+), 11 deletions(-) diff --git a/source/extensions/filters/http/transformation/inja_transformer.cc b/source/extensions/filters/http/transformation/inja_transformer.cc index ed09a0e1..810712df 100644 --- a/source/extensions/filters/http/transformation/inja_transformer.cc +++ b/source/extensions/filters/http/transformation/inja_transformer.cc @@ -1072,8 +1072,15 @@ void InjaTransformer::transform(Http::RequestOrResponseHeaderMap &header_map, // Span transform: if (span_name_template_.has_value()) { - std::string output = instance_->render(span_name_template_.value()); - callbacks.activeSpan().setOperation(output); + // If route.decorator.operation is set, do not update the span name. + if (callbacks.route() + && callbacks.route()->decorator() + && !callbacks.route()->decorator()->getOperation().empty()) { + callbacks.activeSpan().setOperation(callbacks.route()->decorator()->getOperation()); + } else { + std::string output = instance_->render(span_name_template_.value()); + callbacks.activeSpan().setOperation(output); + } } // replace body. we do it here so that headers and dynamic metadata have the diff --git a/test/extensions/filters/http/transformation/inja_transformer_test.cc b/test/extensions/filters/http/transformation/inja_transformer_test.cc index c737fde7..08733982 100644 --- a/test/extensions/filters/http/transformation/inja_transformer_test.cc +++ b/test/extensions/filters/http/transformation/inja_transformer_test.cc @@ -1238,22 +1238,65 @@ TEST_F(InjaTransformerTest, ParseFromClusterMetadata) { EXPECT_EQ(body.toString(), "val"); } -TEST_F(InjaTransformerTest, SetSpanName) { - using ::envoy::api::v2::filter::http::TransformationTemplate_SpanTransformer; - std::string test_span_name = "TEST_SPAN_NAME"; - Http::TestRequestHeaderMapImpl headers{ - {":method", "GET"}, - {":path", "/"}, - }; +TEST_F(InjaTransformerTest, SetSpanNameNullRouteDecorator) { + std::string transformer_span_name = "TRANSFORMER_SPAN_NAME"; + TransformationTemplate transformation; + transformation.mutable_span_transformer()->mutable_name()->set_text(transformer_span_name); + + Http::TestRequestHeaderMapImpl headers{}; + Buffer::OwnedImpl body(""); + NiceMock callbacks; + + InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_); + std::unique_ptr mock_span = std::make_unique(); + const std::unique_ptr mock_decorator = std::make_unique>(); + ON_CALL(*callbacks.route_, decorator).WillByDefault(Return(nullptr)); + EXPECT_CALL(callbacks, activeSpan).WillOnce(ReturnRef(*mock_span)); + EXPECT_CALL(*mock_span, setOperation(transformer_span_name)).Times(1); + + transformer.transform(headers, &headers, body, callbacks); +} + +TEST_F(InjaTransformerTest, SetSpanNameEmptyRouteDecorator) { + std::string transformer_span_name = "TRANSFORMER_SPAN_NAME"; TransformationTemplate transformation; - transformation.mutable_span_transformer()->mutable_name()->set_text(test_span_name); + transformation.mutable_span_transformer()->mutable_name()->set_text(transformer_span_name); + Http::TestRequestHeaderMapImpl headers{}; + Buffer::OwnedImpl body(""); NiceMock callbacks; + InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_); + std::unique_ptr mock_span = std::make_unique(); + const std::unique_ptr mock_decorator = std::make_unique>(); + EXPECT_CALL(*callbacks.route_, decorator).WillRepeatedly(Return(mock_decorator.get())); + ON_CALL(*mock_decorator, getOperation()).WillByDefault(ReturnRef("")); + EXPECT_CALL(callbacks, activeSpan).WillOnce(ReturnRef(*mock_span)); + EXPECT_CALL(*mock_span, setOperation(transformer_span_name)).Times(1); + + transformer.transform(headers, &headers, body, callbacks); +} + +TEST_F(InjaTransformerTest, SetSpanNameNonEmptyRouteDecorator) { + // Ensure that if route->decorator->operation is set, that it overrides the + // transformer value + std::string transformer_span_name = "TRANSFORMER_SPAN_NAME"; + std::string decorator_span_Name = "DECORATOR_SPAN_NAME"; + TransformationTemplate transformation; + transformation.mutable_span_transformer()->mutable_name()->set_text(transformer_span_name); + + Http::TestRequestHeaderMapImpl headers{}; Buffer::OwnedImpl body(""); + NiceMock callbacks; + + InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_); std::unique_ptr mock_span = std::make_unique(); + const std::unique_ptr mock_decorator = std::make_unique>(); + EXPECT_CALL(*callbacks.route_, decorator).WillRepeatedly(Return(mock_decorator.get())); + ON_CALL(*mock_decorator, getOperation()).WillByDefault(ReturnRef(decorator_span_Name)); EXPECT_CALL(callbacks, activeSpan).WillOnce(ReturnRef(*mock_span)); - EXPECT_CALL(*mock_span, setOperation(test_span_name)).Times(1); + EXPECT_CALL(*mock_span, setOperation(decorator_span_Name)).Times(1); + transformer.transform(headers, &headers, body, callbacks); } From 2e697111621916e6a9d8f985760cb14d83ebfbc4 Mon Sep 17 00:00:00 2001 From: Ashish Banerjee Date: Mon, 30 Sep 2024 20:28:30 +0000 Subject: [PATCH 8/9] Add test for null route in callbacks --- .../transformation/inja_transformer_test.cc | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/extensions/filters/http/transformation/inja_transformer_test.cc b/test/extensions/filters/http/transformation/inja_transformer_test.cc index 08733982..e72c51e0 100644 --- a/test/extensions/filters/http/transformation/inja_transformer_test.cc +++ b/test/extensions/filters/http/transformation/inja_transformer_test.cc @@ -1238,6 +1238,25 @@ TEST_F(InjaTransformerTest, ParseFromClusterMetadata) { EXPECT_EQ(body.toString(), "val"); } +TEST_F(InjaTransformerTest, SetSpanNameNullRoute) { + std::string transformer_span_name = "TRANSFORMER_SPAN_NAME"; + TransformationTemplate transformation; + transformation.mutable_span_transformer()->mutable_name()->set_text(transformer_span_name); + + Http::TestRequestHeaderMapImpl headers{}; + Buffer::OwnedImpl body(""); + NiceMock callbacks; + + InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_); + std::unique_ptr mock_span = std::make_unique(); + const std::unique_ptr mock_decorator = std::make_unique>(); + ON_CALL(callbacks, route).WillByDefault(Return(nullptr)); + EXPECT_CALL(callbacks, activeSpan).WillOnce(ReturnRef(*mock_span)); + EXPECT_CALL(*mock_span, setOperation(transformer_span_name)).Times(1); + + transformer.transform(headers, &headers, body, callbacks); +} + TEST_F(InjaTransformerTest, SetSpanNameNullRouteDecorator) { std::string transformer_span_name = "TRANSFORMER_SPAN_NAME"; TransformationTemplate transformation; From 3d7463c0748ecbc6ec3dcf309a5b4fd19d9b81df Mon Sep 17 00:00:00 2001 From: Ashish Banerjee Date: Tue, 1 Oct 2024 15:25:33 +0000 Subject: [PATCH 9/9] Remove redundant call to setOperation This is the default value that is set from the HCM - we do not need to write it a second time. --- .../filters/http/transformation/inja_transformer.cc | 7 +++---- .../filters/http/transformation/inja_transformer_test.cc | 4 +--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/source/extensions/filters/http/transformation/inja_transformer.cc b/source/extensions/filters/http/transformation/inja_transformer.cc index 810712df..d1a4e557 100644 --- a/source/extensions/filters/http/transformation/inja_transformer.cc +++ b/source/extensions/filters/http/transformation/inja_transformer.cc @@ -1073,11 +1073,10 @@ void InjaTransformer::transform(Http::RequestOrResponseHeaderMap &header_map, // Span transform: if (span_name_template_.has_value()) { // If route.decorator.operation is set, do not update the span name. - if (callbacks.route() + bool route_has_decorator_operation = callbacks.route() && callbacks.route()->decorator() - && !callbacks.route()->decorator()->getOperation().empty()) { - callbacks.activeSpan().setOperation(callbacks.route()->decorator()->getOperation()); - } else { + && !callbacks.route()->decorator()->getOperation().empty(); + if (!route_has_decorator_operation) { std::string output = instance_->render(span_name_template_.value()); callbacks.activeSpan().setOperation(output); } diff --git a/test/extensions/filters/http/transformation/inja_transformer_test.cc b/test/extensions/filters/http/transformation/inja_transformer_test.cc index e72c51e0..8eb8745b 100644 --- a/test/extensions/filters/http/transformation/inja_transformer_test.cc +++ b/test/extensions/filters/http/transformation/inja_transformer_test.cc @@ -1309,12 +1309,10 @@ TEST_F(InjaTransformerTest, SetSpanNameNonEmptyRouteDecorator) { NiceMock callbacks; InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_); - std::unique_ptr mock_span = std::make_unique(); const std::unique_ptr mock_decorator = std::make_unique>(); EXPECT_CALL(*callbacks.route_, decorator).WillRepeatedly(Return(mock_decorator.get())); ON_CALL(*mock_decorator, getOperation()).WillByDefault(ReturnRef(decorator_span_Name)); - EXPECT_CALL(callbacks, activeSpan).WillOnce(ReturnRef(*mock_span)); - EXPECT_CALL(*mock_span, setOperation(decorator_span_Name)).Times(1); + EXPECT_CALL(callbacks, activeSpan).Times(0); transformer.transform(headers, &headers, body, callbacks); }