Convert Span::setTag and Span::setOperation to string_view (#6817)

Description: Convert `Span::setTag` and `Span::setOperation` to  take `absl::string_view` arguments. This avoids unnecessary string construction when the value string originates from a header. Zipkin ultimately requires string construction in some cases, but we can directly convert to an Opentracing `string_view` flavor without any additional construction.
Risk Level: Low
Testing: `bazel test //test/...`
Docs Changes: N/A
Release Notes: N/A
Part of: Issue #6580

Signed-off-by: Dan Noé <dpn@google.com>

include/envoy/tracing/http_tracer.h

@@ -74,14 +74,14 @@ class Span {
    * Set the operation name.
    * @param operation the operation name
    */
-  virtual void setOperation(const std::string& operation) PURE;
+  virtual void setOperation(absl::string_view operation) PURE;
 
   /**
    * Attach metadata to a Span, to be handled in an implementation-dependent fashion.
    * @param name the name of the tag
    * @param value the value to associate with the tag
    */
-  virtual void setTag(const std::string& name, const std::string& value) PURE;
+  virtual void setTag(absl::string_view name, absl::string_view value) PURE;
 
   /**
    * Record an event associated with a span, to be handled in an implementation-dependent fashion.

source/common/grpc/common.cc

@@ -57,7 +57,6 @@ void Common::chargeStat(const Upstream::ClusterInfo& cluster, const std::string&
                            grpc_status->value().getStringView()))
       .inc();
   uint64_t grpc_status_code;
-  // TODO(dnoe): Migrate to pure string_view (#6580)
   const bool success = absl::SimpleAtoi(grpc_status->value().getStringView(), &grpc_status_code) &&
                        grpc_status_code == 0;
   chargeStat(cluster, protocol, grpc_service, grpc_method, success);

source/common/http/conn_manager_impl.cc

@@ -800,8 +800,7 @@ void ConnectionManagerImpl::ActiveStream::traceRequest() {
     // should be used to override the active span's operation.
     if (req_operation_override) {
       if (!req_operation_override->value().empty()) {
-        // TODO(dnoe): Migrate setOperation to take string_view (#6580)
-        active_span_->setOperation(std::string(req_operation_override->value().getStringView()));
+        active_span_->setOperation(req_operation_override->value().getStringView());
 
         // Clear the decorated operation so won't be used in the response header, as
         // it has been overridden by the inbound decorator operation request header.
@@ -1313,7 +1312,7 @@ void ConnectionManagerImpl::ActiveStream::encodeHeaders(ActiveStreamEncoderFilte
       // should be used to override the active span's operation.
       if (resp_operation_override) {
         if (!resp_operation_override->value().empty() && active_span_) {
-          active_span_->setOperation(std::string(resp_operation_override->value().getStringView()));
+          active_span_->setOperation(resp_operation_override->value().getStringView());
         }
         // Remove header so not propagated to service.
         headers.removeEnvoyDecoratorOperation();

source/common/tracing/http_tracer_impl.cc

@@ -147,7 +147,7 @@ void HttpTracerUtility::finalizeSpan(Span& span, const Http::HeaderMap* request_
     for (const Http::LowerCaseString& header : tracing_config.requestHeadersForTags()) {
       const Http::HeaderEntry* entry = request_headers->get(header);
       if (entry) {
-        span.setTag(header.get(), std::string(entry->value().getStringView()));
+        span.setTag(header.get(), entry->value().getStringView());
       }
     }
   }

source/common/tracing/http_tracer_impl.h

@@ -130,8 +130,8 @@ class NullSpan : public Span {
   }
 
   // Tracing::Span
-  void setOperation(const std::string&) override {}
-  void setTag(const std::string&, const std::string&) override {}
+  void setOperation(absl::string_view) override {}
+  void setTag(absl::string_view, absl::string_view) override {}
   void log(SystemTime, const std::string&) override {}
   void finishSpan() override {}
   void injectContext(Http::HeaderMap&) override {}

source/extensions/tracers/common/ot/opentracing_driver_impl.cc

@@ -91,12 +91,13 @@ OpenTracingSpan::OpenTracingSpan(OpenTracingDriver& driver,
 
 void OpenTracingSpan::finishSpan() { span_->FinishWithOptions(finish_options_); }
 
-void OpenTracingSpan::setOperation(const std::string& operation) {
-  span_->SetOperationName(operation);
+void OpenTracingSpan::setOperation(absl::string_view operation) {
+  span_->SetOperationName({operation.data(), operation.length()});
 }
 
-void OpenTracingSpan::setTag(const std::string& name, const std::string& value) {
-  span_->SetTag(name, value);
+void OpenTracingSpan::setTag(absl::string_view name, absl::string_view value) {
+  span_->SetTag({name.data(), name.length()},
+                opentracing::v2::string_view{value.data(), value.length()});
 }
 
 void OpenTracingSpan::log(SystemTime timestamp, const std::string& event) {

source/extensions/tracers/common/ot/opentracing_driver_impl.h

@@ -33,8 +33,8 @@ class OpenTracingSpan : public Tracing::Span, Logger::Loggable<Logger::Id::traci
 
   // Tracing::Span
   void finishSpan() override;
-  void setOperation(const std::string& operation) override;
-  void setTag(const std::string& name, const std::string& value) override;
+  void setOperation(absl::string_view operation) override;
+  void setTag(absl::string_view name, const absl::string_view) override;
   void log(SystemTime timestamp, const std::string& event) override;
   void injectContext(Http::HeaderMap& request_headers) override;
   Tracing::SpanPtr spawnChild(const Tracing::Config& config, const std::string& name,

source/extensions/tracers/zipkin/zipkin_core_types.cc

@@ -252,7 +252,7 @@ void Span::finish() {
   }
 }
 
-void Span::setTag(const std::string& name, const std::string& value) {
+void Span::setTag(absl::string_view name, absl::string_view value) {
   if (!name.empty() && !value.empty()) {
     addBinaryAnnotation(BinaryAnnotation(name, value));
   }

source/extensions/tracers/zipkin/zipkin_core_types.h

@@ -11,6 +11,7 @@
 #include "extensions/tracers/zipkin/tracer_interface.h"
 #include "extensions/tracers/zipkin/util.h"
 
+#include "absl/strings/string_view.h"
 #include "absl/types/optional.h"
 
 namespace Envoy {
@@ -224,7 +225,7 @@ class BinaryAnnotation : public ZipkinBase {
    * @param key The key name of the annotation.
    * @param value The value associated with the key.
    */
-  BinaryAnnotation(const std::string& key, const std::string& value)
+  BinaryAnnotation(absl::string_view key, absl::string_view value)
       : key_(key), value_(value), annotation_type_(STRING) {}
 
   /**
@@ -547,7 +548,7 @@ class Span : public ZipkinBase {
    * @param name The binary annotation's key.
    * @param value The binary annotation's value.
    */
-  void setTag(const std::string& name, const std::string& value);
+  void setTag(absl::string_view name, absl::string_view value);
 
   /**
    * Adds an annotation to the span

source/extensions/tracers/zipkin/zipkin_tracer_impl.cc

@@ -22,9 +22,11 @@ ZipkinSpan::ZipkinSpan(Zipkin::Span& span, Zipkin::Tracer& tracer) : span_(span)
 
 void ZipkinSpan::finishSpan() { span_.finish(); }
 
-void ZipkinSpan::setOperation(const std::string& operation) { span_.setName(operation); }
+void ZipkinSpan::setOperation(absl::string_view operation) {
+  span_.setName(std::string(operation));
+}
 
-void ZipkinSpan::setTag(const std::string& name, const std::string& value) {
+void ZipkinSpan::setTag(absl::string_view name, absl::string_view value) {
   span_.setTag(name, value);
 }
 

source/extensions/tracers/zipkin/zipkin_tracer_impl.h

@@ -50,7 +50,7 @@ class ZipkinSpan : public Tracing::Span {
    * This method sets the operation name on the span.
    * @param operation the operation name
    */
-  void setOperation(const std::string& operation) override;
+  void setOperation(absl::string_view operation) override;
 
   /**
    * This function adds a Zipkin "string" binary annotation to this span.
@@ -60,7 +60,7 @@ class ZipkinSpan : public Tracing::Span {
    * Note that Tracing::HttpTracerUtility::finalizeSpan() makes several calls to this function,
    * associating several key-value pairs with this span.
    */
-  void setTag(const std::string& name, const std::string& value) override;
+  void setTag(absl::string_view name, absl::string_view value) override;
 
   void log(SystemTime timestamp, const std::string& event) override;
 

test/common/grpc/async_client_impl_test.cc

@@ -10,6 +10,7 @@
 #include "gtest/gtest.h"
 
 using testing::_;
+using testing::Eq;
 using testing::Invoke;
 using testing::Return;
 using testing::ReturnRef;
@@ -58,10 +59,11 @@ TEST_F(EnvoyAsyncClientImplTest, RequestHttpStartFail) {
   Tracing::MockSpan* child_span{new Tracing::MockSpan()};
   EXPECT_CALL(active_span, spawnChild_(_, "async test_cluster egress", _))
       .WillOnce(Return(child_span));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().Component, Tracing::Tags::get().Proxy));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().UpstreamCluster, "test_cluster"));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().GrpcStatusCode, "14"));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True));
+  EXPECT_CALL(*child_span,
+              setTag(Eq(Tracing::Tags::get().Component), Eq(Tracing::Tags::get().Proxy)));
+  EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), Eq("test_cluster")));
+  EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("14")));
+  EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
   EXPECT_CALL(*child_span, finishSpan());
   EXPECT_CALL(*child_span, injectContext(_)).Times(0);
 
@@ -123,11 +125,12 @@ TEST_F(EnvoyAsyncClientImplTest, RequestHttpSendHeadersFail) {
   Tracing::MockSpan* child_span{new Tracing::MockSpan()};
   EXPECT_CALL(active_span, spawnChild_(_, "async test_cluster egress", _))
       .WillOnce(Return(child_span));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().Component, Tracing::Tags::get().Proxy));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().UpstreamCluster, "test_cluster"));
+  EXPECT_CALL(*child_span,
+              setTag(Eq(Tracing::Tags::get().Component), Eq(Tracing::Tags::get().Proxy)));
+  EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), Eq("test_cluster")));
   EXPECT_CALL(*child_span, injectContext(_));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().GrpcStatusCode, "13"));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True));
+  EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("13")));
+  EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
   EXPECT_CALL(*child_span, finishSpan());
 
   auto* grpc_request = grpc_client_->send(*method_descriptor_, request_msg, grpc_callbacks,

test/common/grpc/google_async_client_impl_test.cc

@@ -15,6 +15,7 @@
 #include "gtest/gtest.h"
 
 using testing::_;
+using testing::Eq;
 using testing::Return;
 
 namespace Envoy {
@@ -95,10 +96,11 @@ TEST_F(EnvoyGoogleAsyncClientImplTest, RequestHttpStartFail) {
   Tracing::MockSpan* child_span{new Tracing::MockSpan()};
   EXPECT_CALL(active_span, spawnChild_(_, "async test_cluster egress", _))
       .WillOnce(Return(child_span));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().Component, Tracing::Tags::get().Proxy));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().UpstreamCluster, "test_cluster"));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().GrpcStatusCode, "14"));
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True));
+  EXPECT_CALL(*child_span,
+              setTag(Eq(Tracing::Tags::get().Component), Eq(Tracing::Tags::get().Proxy)));
+  EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), Eq("test_cluster")));
+  EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("14")));
+  EXPECT_CALL(*child_span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
   EXPECT_CALL(*child_span, finishSpan());
   EXPECT_CALL(*child_span, injectContext(_));
 

test/common/grpc/grpc_client_integration_test.cc

@@ -7,6 +7,8 @@
 
 #include "test/common/grpc/grpc_client_integration_test_harness.h"
 
+using testing::Eq;
+
 namespace Envoy {
 namespace Grpc {
 namespace {
@@ -272,8 +274,9 @@ TEST_P(GrpcClientIntegrationTest, RequestTrailersOnly) {
   initialize();
   auto request = createRequest(empty_metadata_);
   const Http::TestHeaderMapImpl reply_headers{{":status", "200"}, {"grpc-status", "0"}};
-  EXPECT_CALL(*request->child_span_, setTag(Tracing::Tags::get().GrpcStatusCode, "0"));
-  EXPECT_CALL(*request->child_span_, setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True));
+  EXPECT_CALL(*request->child_span_, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("0")));
+  EXPECT_CALL(*request->child_span_,
+              setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True)));
   EXPECT_CALL(*request, onFailure(Status::Internal, "", _)).WillExitIfNeeded();
   dispatcher_helper_.setStreamEventPending();
   EXPECT_CALL(*request->child_span_, finishSpan());
@@ -340,7 +343,7 @@ TEST_P(GrpcClientIntegrationTest, CancelRequest) {
   initialize();
   auto request = createRequest(empty_metadata_);
   EXPECT_CALL(*request->child_span_,
-              setTag(Tracing::Tags::get().Status, Tracing::Tags::get().Canceled));
+              setTag(Eq(Tracing::Tags::get().Status), Eq(Tracing::Tags::get().Canceled)));
   EXPECT_CALL(*request->child_span_, finishSpan());
   request->grpc_request_->cancel();
   dispatcher_helper_.dispatcher_.run(Event::Dispatcher::RunType::NonBlock);

test/common/grpc/grpc_client_integration_test_harness.h

@@ -35,6 +35,7 @@
 #include "test/test_common/utility.h"
 
 using testing::_;
+using testing::Eq;
 using testing::Invoke;
 using testing::InvokeWithoutArgs;
 using testing::NiceMock;
@@ -202,7 +203,7 @@ class HelloworldRequest : public MockAsyncRequestCallbacks<helloworld::HelloRepl
     fake_stream_->startGrpcStream();
     helloworld::HelloReply reply;
     reply.set_message(HELLO_REPLY);
-    EXPECT_CALL(*child_span_, setTag(Tracing::Tags::get().GrpcStatusCode, "0"));
+    EXPECT_CALL(*child_span_, setTag(Eq(Tracing::Tags::get().GrpcStatusCode), Eq("0")));
     EXPECT_CALL(*this, onSuccess_(HelloworldReplyEq(HELLO_REPLY), _)).WillExitIfNeeded();
     EXPECT_CALL(*child_span_, finishSpan());
     dispatcher_helper_.setStreamEventPending();
@@ -349,9 +350,9 @@ class GrpcClientIntegrationTest : public GrpcClientIntegrationParamTest {
     EXPECT_CALL(active_span, spawnChild_(_, "async fake_cluster egress", _))
         .WillOnce(Return(request->child_span_));
     EXPECT_CALL(*request->child_span_,
-                setTag(Tracing::Tags::get().UpstreamCluster, fake_cluster_name_));
+                setTag(Eq(Tracing::Tags::get().UpstreamCluster), Eq(fake_cluster_name_)));
     EXPECT_CALL(*request->child_span_,
-                setTag(Tracing::Tags::get().Component, Tracing::Tags::get().Proxy));
+                setTag(Eq(Tracing::Tags::get().Component), Eq(Tracing::Tags::get().Proxy)));
     EXPECT_CALL(*request->child_span_, injectContext(_));
 
     request->grpc_request_ =

test/common/http/conn_manager_impl_test.cc

@@ -48,6 +48,7 @@ using testing::_;
 using testing::AnyNumber;
 using testing::AtLeast;
 using testing::DoAll;
+using testing::Eq;
 using testing::InSequence;
 using testing::Invoke;
 using testing::InvokeWithoutArgs;
@@ -691,9 +692,9 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlow) {
   EXPECT_CALL(*span, finishSpan());
   EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber());
   // Verify tag is set based on the request headers.
-  EXPECT_CALL(*span, setTag(":method", "GET"));
+  EXPECT_CALL(*span, setTag(Eq(":method"), Eq("GET")));
   // Verify if the activeSpan interface returns reference to the current span.
-  EXPECT_CALL(*span, setTag("service-cluster", "scoobydoo"));
+  EXPECT_CALL(*span, setTag(Eq("service-cluster"), Eq("scoobydoo")));
   EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _))
       .WillOnce(Return(true));
   EXPECT_CALL(*span, setOperation(_)).Times(0);
@@ -823,7 +824,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowIngressDecorat
   EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber());
   EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _))
       .WillOnce(Return(true));
-  EXPECT_CALL(*span, setOperation("testOp"));
+  EXPECT_CALL(*span, setOperation(Eq("testOp")));
 
   std::shared_ptr<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
 
@@ -958,7 +959,7 @@ TEST_F(HttpConnectionManagerImplTest, StartAndFinishSpanNormalFlowEgressDecorato
   EXPECT_CALL(runtime_.snapshot_, featureEnabled("tracing.global_enabled", 100, _))
       .WillOnce(Return(true));
   // Verify that span operation overridden by value supplied in response header.
-  EXPECT_CALL(*span, setOperation("testOp"));
+  EXPECT_CALL(*span, setOperation(Eq("testOp")));
 
   std::shared_ptr<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
 

test/common/router/config_impl_test.cc

@@ -32,6 +32,7 @@
 using testing::_;
 using testing::ContainerEq;
 using testing::ElementsAreArray;
+using testing::Eq;
 using testing::Matcher;
 using testing::MockFunction;
 using testing::NiceMock;
@@ -4321,7 +4322,7 @@ TEST_F(RouteMatcherTest, Decorator) {
     Http::TestHeaderMapImpl headers = genHeaders("www.lyft.com", "/foo", "GET");
     Router::RouteConstSharedPtr route = config.route(headers, 0);
     Tracing::MockSpan span;
-    EXPECT_CALL(span, setOperation("myFoo"));
+    EXPECT_CALL(span, setOperation(Eq("myFoo")));
     route->decorator()->apply(span);
   }
   {

test/common/router/router_test.cc

@@ -35,6 +35,7 @@ using testing::AssertionFailure;
 using testing::AssertionResult;
 using testing::AssertionSuccess;
 using testing::AtLeast;
+using testing::Eq;
 using testing::InSequence;
 using testing::Invoke;
 using testing::Matcher;
@@ -3151,7 +3152,8 @@ TEST_F(RouterTestChildSpan, BasicFlow) {
   EXPECT_CALL(callbacks_.active_span_, spawnChild_(_, "router fake_cluster egress", _))
       .WillOnce(Return(child_span));
   EXPECT_CALL(callbacks_, tracingConfig());
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().Component, Tracing::Tags::get().Proxy));
+  EXPECT_CALL(*child_span,
+              setTag(Eq(Tracing::Tags::get().Component), Eq(Tracing::Tags::get().Proxy)));
   router_.decodeHeaders(headers, true);
 
   Http::HeaderMapPtr response_headers(new Http::TestHeaderMapImpl{{":status", "200"}});
@@ -3182,7 +3184,8 @@ TEST_F(RouterTestChildSpan, ResetFlow) {
   EXPECT_CALL(callbacks_.active_span_, spawnChild_(_, "router fake_cluster egress", _))
       .WillOnce(Return(child_span));
   EXPECT_CALL(callbacks_, tracingConfig());
-  EXPECT_CALL(*child_span, setTag(Tracing::Tags::get().Component, Tracing::Tags::get().Proxy));
+  EXPECT_CALL(*child_span,
+              setTag(Eq(Tracing::Tags::get().Component), Eq(Tracing::Tags::get().Proxy)));
   router_.decodeHeaders(headers, true);
 
   Http::HeaderMapPtr response_headers(new Http::TestHeaderMapImpl{{":status", "200"}});