[main up-port] Set opentelemetry span name in transformation filter (#…

…368) (#369)

* Set opentelemetry span name in transformation filter (#368)

* Add setOperation patch

* Add changelog

* Transformation initial implementation

Should work but does not seem to at the moment

* Implement unit test for setting span name

* Update changelog

* Remove extra entry from changelog

* Do not override route.decorator.operation

* Add test for null route in callbacks

* 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.

* Move changelog

* Move changelog... again!

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

changelog/v1.31.2-patch2/opentelemetry-set-operation-patch.yaml

@@ -0,0 +1,6 @@
+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.

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<Buffer::OwnedImpl> maybe_body;
 
@@ -1063,6 +1070,18 @@ 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.
+    bool route_has_decorator_operation = callbacks.route()
+        && callbacks.route()->decorator()
+        && !callbacks.route()->decorator()->getOperation().empty();
+    if (!route_has_decorator_operation) {
+      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()) {

source/extensions/filters/http/transformation/inja_transformer.h

@@ -155,6 +155,7 @@ class InjaTransformer : public Transformer, Logger::Loggable<Logger::Id::filter>
   bool escape_characters_{};
 
   absl::optional<inja::Template> body_template_;
+  absl::optional<inja::Template> 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

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",
     ],
 )
 

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,85 @@ 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<Http::MockStreamDecoderFilterCallbacks> callbacks;
+
+  InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_);
+  std::unique_ptr<Tracing::MockSpan> mock_span = std::make_unique<Tracing::MockSpan>();
+  const std::unique_ptr<Router::MockDecorator> mock_decorator = std::make_unique<NiceMock<Router::MockDecorator>>();
+  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;
+  transformation.mutable_span_transformer()->mutable_name()->set_text(transformer_span_name);
+
+  Http::TestRequestHeaderMapImpl headers{};
+  Buffer::OwnedImpl body("");
+  NiceMock<Http::MockStreamDecoderFilterCallbacks> callbacks;
+
+  InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_);
+  std::unique_ptr<Tracing::MockSpan> mock_span = std::make_unique<Tracing::MockSpan>();
+  const std::unique_ptr<Router::MockDecorator> mock_decorator = std::make_unique<NiceMock<Router::MockDecorator>>();
+  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(transformer_span_name);
+
+  Http::TestRequestHeaderMapImpl headers{};
+  Buffer::OwnedImpl body("");
+  NiceMock<Http::MockStreamDecoderFilterCallbacks> callbacks;
+
+  InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_);
+  std::unique_ptr<Tracing::MockSpan> mock_span = std::make_unique<Tracing::MockSpan>();
+  const std::unique_ptr<Router::MockDecorator> mock_decorator = std::make_unique<NiceMock<Router::MockDecorator>>();
+  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<Http::MockStreamDecoderFilterCallbacks> callbacks;
+
+  InjaTransformer transformer(transformation, rng_, google::protobuf::BoolValue(), tls_);
+  const std::unique_ptr<Router::MockDecorator> mock_decorator = std::make_unique<NiceMock<Router::MockDecorator>>();
+  EXPECT_CALL(*callbacks.route_, decorator).WillRepeatedly(Return(mock_decorator.get()));
+  ON_CALL(*mock_decorator, getOperation()).WillByDefault(ReturnRef(decorator_span_Name));
+  EXPECT_CALL(callbacks, activeSpan).Times(0);
+
+  transformer.transform(headers, &headers, body, callbacks);
+}
+
 const std::string NESTED_KEY =
     R"EOF(
 {
@@ -1246,7 +1326,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 +1360,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\")}}");