From 4aec400e9c61c7ee03ea5e6cb5b045b6a7b5abe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dan=20No=C3=A9?= Date: Tue, 7 May 2019 15:48:34 -0400 Subject: [PATCH] Convert Span::setTag and Span::setOperation to string_view (#6817) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é --- include/envoy/tracing/http_tracer.h | 4 +- source/common/grpc/common.cc | 1 - source/common/http/conn_manager_impl.cc | 5 +- source/common/tracing/http_tracer_impl.cc | 2 +- source/common/tracing/http_tracer_impl.h | 4 +- .../common/ot/opentracing_driver_impl.cc | 9 +- .../common/ot/opentracing_driver_impl.h | 4 +- .../tracers/zipkin/zipkin_core_types.cc | 2 +- .../tracers/zipkin/zipkin_core_types.h | 5 +- .../tracers/zipkin/zipkin_tracer_impl.cc | 6 +- .../tracers/zipkin/zipkin_tracer_impl.h | 4 +- test/common/grpc/async_client_impl_test.cc | 19 ++-- .../grpc/google_async_client_impl_test.cc | 10 +- .../grpc/grpc_client_integration_test.cc | 9 +- .../grpc_client_integration_test_harness.h | 7 +- test/common/http/conn_manager_impl_test.cc | 9 +- test/common/router/config_impl_test.cc | 3 +- test/common/router/router_test.cc | 7 +- test/common/tracing/http_tracer_impl_test.cc | 93 ++++++++++--------- .../ext_authz/ext_authz_grpc_impl_test.cc | 11 ++- .../common/ratelimit/ratelimit_impl_test.cc | 5 +- test/mocks/tracing/mocks.h | 4 +- 22 files changed, 121 insertions(+), 102 deletions(-) diff --git a/include/envoy/tracing/http_tracer.h b/include/envoy/tracing/http_tracer.h index c2574711c346..efb81375dc0e 100644 --- a/include/envoy/tracing/http_tracer.h +++ b/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. diff --git a/source/common/grpc/common.cc b/source/common/grpc/common.cc index b907fe8d3e7e..d2d55bbe62f8 100644 --- a/source/common/grpc/common.cc +++ b/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); diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 4494934e4083..d0b7da544f5f 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/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(); diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 809d43f543c7..46ff74ac58a1 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/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()); } } } diff --git a/source/common/tracing/http_tracer_impl.h b/source/common/tracing/http_tracer_impl.h index d92dc3c84c51..2c557d41cace 100644 --- a/source/common/tracing/http_tracer_impl.h +++ b/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 {} diff --git a/source/extensions/tracers/common/ot/opentracing_driver_impl.cc b/source/extensions/tracers/common/ot/opentracing_driver_impl.cc index 75faf846d914..4b5f5f5b3627 100644 --- a/source/extensions/tracers/common/ot/opentracing_driver_impl.cc +++ b/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) { diff --git a/source/extensions/tracers/common/ot/opentracing_driver_impl.h b/source/extensions/tracers/common/ot/opentracing_driver_impl.h index 72c24c72e680..323afabe56a4 100644 --- a/source/extensions/tracers/common/ot/opentracing_driver_impl.h +++ b/source/extensions/tracers/common/ot/opentracing_driver_impl.h @@ -33,8 +33,8 @@ class OpenTracingSpan : public Tracing::Span, Logger::Loggablesend(*method_descriptor_, request_msg, grpc_callbacks, diff --git a/test/common/grpc/google_async_client_impl_test.cc b/test/common/grpc/google_async_client_impl_test.cc index dd2e98cf2e3d..3389e40a0220 100644 --- a/test/common/grpc/google_async_client_impl_test.cc +++ b/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(_)); diff --git a/test/common/grpc/grpc_client_integration_test.cc b/test/common/grpc/grpc_client_integration_test.cc index d7d85eb45ffc..99c9fe55f1a8 100644 --- a/test/common/grpc/grpc_client_integration_test.cc +++ b/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); diff --git a/test/common/grpc/grpc_client_integration_test_harness.h b/test/common/grpc/grpc_client_integration_test_harness.h index b5045be4171e..4d4a94e288e7 100644 --- a/test/common/grpc/grpc_client_integration_test_harness.h +++ b/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 MockAsyncRequestCallbacksstartGrpcStream(); 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_ = diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index f848584a2d60..bfe7d2a4a9ca 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/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 filter(new NiceMock()); @@ -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 filter(new NiceMock()); diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc index f7a9e36ce6e4..b774faa39c40 100644 --- a/test/common/router/config_impl_test.cc +++ b/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); } { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 1c71f2dfc072..e2bfb3ad3039 100644 --- a/test/common/router/router_test.cc +++ b/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"}}); diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index b939c516483a..4aeee65fc6ac 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -24,6 +24,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::Invoke; using testing::NiceMock; using testing::Return; @@ -131,9 +132,9 @@ TEST(HttpConnManFinalizerImpl, OriginalAndLongPath) { EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); EXPECT_CALL(span, setTag(_, _)).Times(testing::AnyNumber()); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpUrl, path_prefix + expected_path)); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpMethod, "GET")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpProtocol, "HTTP/2")); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpUrl), Eq(path_prefix + expected_path))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("GET"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2"))); NiceMock config; HttpTracerUtility::finalizeSpan(span, &request_headers, stream_info, config); @@ -157,9 +158,9 @@ TEST(HttpConnManFinalizerImpl, NoGeneratedId) { EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); EXPECT_CALL(span, setTag(_, _)).Times(testing::AnyNumber()); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpUrl, path_prefix + expected_path)); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpMethod, "GET")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpProtocol, "HTTP/2")); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpUrl), Eq(path_prefix + expected_path))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("GET"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/2"))); NiceMock config; HttpTracerUtility::finalizeSpan(span, &request_headers, stream_info, config); @@ -175,12 +176,12 @@ TEST(HttpConnManFinalizerImpl, NullRequestHeaders) { EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); EXPECT_CALL(stream_info, upstreamHost()).WillOnce(Return(nullptr)); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpStatusCode, "0")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True)); - EXPECT_CALL(span, setTag(Tracing::Tags::get().ResponseSize, "11")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().ResponseFlags, "-")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().RequestSize, "10")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().UpstreamCluster, _)).Times(0); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("0"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseSize), Eq("11"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseFlags), Eq("-"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().RequestSize), Eq("10"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), _)).Times(0); NiceMock config; HttpTracerUtility::finalizeSpan(span, nullptr, stream_info, config); @@ -235,12 +236,12 @@ TEST(HttpConnManFinalizerImpl, UpstreamClusterTagSet) { EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); EXPECT_CALL(stream_info, upstreamHost()).Times(2); - EXPECT_CALL(span, setTag(Tracing::Tags::get().UpstreamCluster, "my_upstream_cluster")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpStatusCode, "0")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True)); - EXPECT_CALL(span, setTag(Tracing::Tags::get().ResponseSize, "11")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().ResponseFlags, "-")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().RequestSize, "10")); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), Eq("my_upstream_cluster"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("0"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseSize), Eq("11"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseFlags), Eq("-"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().RequestSize), Eq("10"))); NiceMock config; HttpTracerUtility::finalizeSpan(span, nullptr, stream_info, config); @@ -261,24 +262,24 @@ TEST(HttpConnManFinalizerImpl, SpanOptionalHeaders) { const std::string service_node = "i-453"; // Check that span is populated correctly. - EXPECT_CALL(span, setTag(Tracing::Tags::get().GuidXRequestId, "id")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpUrl, "https:///test")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpMethod, "GET")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().UserAgent, "-")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpProtocol, "HTTP/1.0")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().DownstreamCluster, "-")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().RequestSize, "10")); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GuidXRequestId), Eq("id"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpUrl), Eq("https:///test"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("GET"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UserAgent), Eq("-"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/1.0"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().DownstreamCluster), Eq("-"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().RequestSize), Eq("10"))); absl::optional response_code; EXPECT_CALL(stream_info, responseCode()).WillRepeatedly(ReturnPointee(&response_code)); EXPECT_CALL(stream_info, bytesSent()).WillOnce(Return(100)); EXPECT_CALL(stream_info, upstreamHost()).WillOnce(Return(nullptr)); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpStatusCode, "0")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True)); - EXPECT_CALL(span, setTag(Tracing::Tags::get().ResponseSize, "100")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().ResponseFlags, "-")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().UpstreamCluster, _)).Times(0); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("0"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseSize), Eq("100"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseFlags), Eq("-"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), _)).Times(0); NiceMock config; HttpTracerUtility::finalizeSpan(span, &request_headers, stream_info, config); @@ -303,14 +304,14 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { const std::string service_node = "i-453"; // Check that span is populated correctly. - EXPECT_CALL(span, setTag(Tracing::Tags::get().GuidXRequestId, "id")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpUrl, "http://api/test")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpMethod, "GET")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().UserAgent, "agent")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpProtocol, "HTTP/1.0")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().DownstreamCluster, "downstream_cluster")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().RequestSize, "10")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().GuidXClientTraceId, "client_trace_id")); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GuidXRequestId), Eq("id"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpUrl), Eq("http://api/test"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpMethod), Eq("GET"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UserAgent), Eq("agent"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpProtocol), Eq("HTTP/1.0"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().DownstreamCluster), Eq("downstream_cluster"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().RequestSize), Eq("10"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().GuidXClientTraceId), Eq("client_trace_id"))); // Check that span has tags from custom headers. request_headers.addCopy(Http::LowerCaseString("aa"), "a"); @@ -320,8 +321,8 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { config.headers_.push_back(Http::LowerCaseString("aa")); config.headers_.push_back(Http::LowerCaseString("cc")); config.headers_.push_back(Http::LowerCaseString("ee")); - EXPECT_CALL(span, setTag("aa", "a")); - EXPECT_CALL(span, setTag("cc", "c")); + EXPECT_CALL(span, setTag(Eq("aa"), Eq("a"))); + EXPECT_CALL(span, setTag(Eq("cc"), Eq("c"))); EXPECT_CALL(config, requestHeadersForTags()); EXPECT_CALL(config, verbose).WillOnce(Return(false)); @@ -332,11 +333,11 @@ TEST(HttpConnManFinalizerImpl, SpanPopulatedFailureResponse) { .WillByDefault(Return(true)); EXPECT_CALL(stream_info, upstreamHost()).WillOnce(Return(nullptr)); - EXPECT_CALL(span, setTag(Tracing::Tags::get().Error, Tracing::Tags::get().True)); - EXPECT_CALL(span, setTag(Tracing::Tags::get().HttpStatusCode, "503")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().ResponseSize, "100")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().ResponseFlags, "UT")); - EXPECT_CALL(span, setTag(Tracing::Tags::get().UpstreamCluster, _)).Times(0); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().Error), Eq(Tracing::Tags::get().True))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().HttpStatusCode), Eq("503"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseSize), Eq("100"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().ResponseFlags), Eq("UT"))); + EXPECT_CALL(span, setTag(Eq(Tracing::Tags::get().UpstreamCluster), _)).Times(0); HttpTracerUtility::finalizeSpan(span, &request_headers, stream_info, config); } @@ -399,7 +400,7 @@ TEST_F(HttpTracerImplTest, BasicFunctionalityNodeSet) { EXPECT_CALL(*driver_, startSpan_(_, _, operation_name, stream_info_.start_time_, _)) .WillOnce(Return(span)); EXPECT_CALL(*span, setTag(_, _)).Times(testing::AnyNumber()); - EXPECT_CALL(*span, setTag(Tracing::Tags::get().NodeId, "node_name")); + EXPECT_CALL(*span, setTag(Eq(Tracing::Tags::get().NodeId), Eq("node_name"))); tracer_->startSpan(config_, request_headers_, stream_info_, {Reason::Sampling, true}); } diff --git a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc index 1094ac87f0f0..e79910f96878 100644 --- a/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc +++ b/test/extensions/filters/common/ext_authz/ext_authz_grpc_impl_test.cc @@ -14,6 +14,7 @@ #include "gtest/gtest.h" using testing::_; +using testing::Eq; using testing::Invoke; using testing::Ref; using testing::Return; @@ -81,7 +82,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationOk) { Http::HeaderMapImpl headers; client_->onCreateInitialMetadata(headers); - EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_ok")); + EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_ok"))); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( AuthzResponseNoAttributes(authz_response)))); client_->onSuccess(std::move(check_response), span_); @@ -105,7 +106,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationOkWithAllAtributes) { Http::HeaderMapImpl headers; client_->onCreateInitialMetadata(headers); - EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_ok")); + EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_ok"))); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzOkResponse(authz_response)))); client_->onSuccess(std::move(check_response), span_); @@ -128,7 +129,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDenied) { Http::HeaderMapImpl headers; client_->onCreateInitialMetadata(headers); EXPECT_EQ(nullptr, headers.RequestId()); - EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_unauthorized")); + EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_unauthorized"))); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( AuthzResponseNoAttributes(authz_response)))); @@ -152,7 +153,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDeniedGrpcUnknownStatus) { Http::HeaderMapImpl headers; client_->onCreateInitialMetadata(headers); EXPECT_EQ(nullptr, headers.RequestId()); - EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_unauthorized")); + EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_unauthorized"))); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo( AuthzResponseNoAttributes(authz_response)))); @@ -179,7 +180,7 @@ TEST_P(ExtAuthzGrpcClientTest, AuthorizationDeniedWithAllAttributes) { Http::HeaderMapImpl headers; client_->onCreateInitialMetadata(headers); EXPECT_EQ(nullptr, headers.RequestId()); - EXPECT_CALL(span_, setTag("ext_authz_status", "ext_authz_unauthorized")); + EXPECT_CALL(span_, setTag(Eq("ext_authz_status"), Eq("ext_authz_unauthorized"))); EXPECT_CALL(request_callbacks_, onComplete_(WhenDynamicCastTo(AuthzDeniedResponse(authz_response)))); diff --git a/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc b/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc index 8ffff0462ba1..d643bb5396d7 100644 --- a/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc +++ b/test/extensions/filters/common/ratelimit/ratelimit_impl_test.cc @@ -21,6 +21,7 @@ using testing::_; using testing::AtLeast; +using testing::Eq; using testing::Invoke; using testing::Ref; using testing::Return; @@ -80,7 +81,7 @@ TEST_F(RateLimitGrpcClientTest, Basic) { response = std::make_unique(); response->set_overall_code(envoy::service::ratelimit::v2::RateLimitResponse_Code_OVER_LIMIT); - EXPECT_CALL(span_, setTag("ratelimit_status", "over_limit")); + EXPECT_CALL(span_, setTag(Eq("ratelimit_status"), Eq("over_limit"))); EXPECT_CALL(request_callbacks_, complete_(LimitStatus::OverLimit, _)); client_.onSuccess(std::move(response), span_); } @@ -99,7 +100,7 @@ TEST_F(RateLimitGrpcClientTest, Basic) { response = std::make_unique(); response->set_overall_code(envoy::service::ratelimit::v2::RateLimitResponse_Code_OK); - EXPECT_CALL(span_, setTag("ratelimit_status", "ok")); + EXPECT_CALL(span_, setTag(Eq("ratelimit_status"), Eq("ok"))); EXPECT_CALL(request_callbacks_, complete_(LimitStatus::OK, _)); client_.onSuccess(std::move(response), span_); } diff --git a/test/mocks/tracing/mocks.h b/test/mocks/tracing/mocks.h index 71ebb49d0e5c..d62036fd8d07 100644 --- a/test/mocks/tracing/mocks.h +++ b/test/mocks/tracing/mocks.h @@ -29,8 +29,8 @@ class MockSpan : public Span { MockSpan(); ~MockSpan(); - MOCK_METHOD1(setOperation, void(const std::string& operation)); - MOCK_METHOD2(setTag, void(const std::string& name, const std::string& value)); + MOCK_METHOD1(setOperation, void(absl::string_view operation)); + MOCK_METHOD2(setTag, void(absl::string_view name, absl::string_view value)); MOCK_METHOD2(log, void(SystemTime timestamp, const std::string& event)); MOCK_METHOD0(finishSpan, void()); MOCK_METHOD1(injectContext, void(Http::HeaderMap& request_headers));