From ebe084bfc67c08f6d5aaf857dd7874729d9f5908 Mon Sep 17 00:00:00 2001 From: Larry Osterman Date: Wed, 1 Jun 2022 17:20:26 -0700 Subject: [PATCH] OpenTelemetry API Review Feedback (#3687) * OpenTelemetry API Review Feedback --- .../test/ut/service_support_test.cpp | 50 +++++++++- .../core/internal/tracing/service_tracing.hpp | 14 ++- .../src/http/request_activity_policy.cpp | 98 ++++++++++--------- sdk/core/azure-core/src/tracing/tracing.cpp | 70 ++++++------- .../test/ut/request_activity_policy_test.cpp | 64 +++++++----- .../test/ut/service_tracing_test.cpp | 25 +++++ 6 files changed, 208 insertions(+), 113 deletions(-) diff --git a/sdk/core/azure-core-tracing-opentelemetry/test/ut/service_support_test.cpp b/sdk/core/azure-core-tracing-opentelemetry/test/ut/service_support_test.cpp index 38b0a42d1..377ba2e66 100644 --- a/sdk/core/azure-core-tracing-opentelemetry/test/ut/service_support_test.cpp +++ b/sdk/core/azure-core-tracing-opentelemetry/test/ut/service_support_test.cpp @@ -391,6 +391,54 @@ TEST_F(OpenTelemetryServiceTests, CreateWithImplicitProvider) Azure::Core::Context::ApplicationContext.SetTracerProvider(nullptr); } +TEST_F(OpenTelemetryServiceTests, CreateSpanWithOptions) +{ + { + auto tracerProvider(CreateOpenTelemetryProvider()); + auto provider(std::make_shared( + tracerProvider)); + + Azure::Core::Context::ApplicationContext.SetTracerProvider(provider); + + { + Azure::Core::_internal::ClientOptions clientOptions; + clientOptions.Telemetry.ApplicationId = "MyApplication"; + + Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace( + clientOptions, "my-service", "1.0beta-2"); + + Azure::Core::Context clientContext; + Azure::Core::Tracing::_internal::CreateSpanOptions createOptions; + createOptions.Kind = Azure::Core::Tracing::_internal::SpanKind::Internal; + createOptions.Attributes = serviceTrace.CreateAttributeSet(); + createOptions.Attributes->AddAttribute("TestAttribute", 3); + auto contextAndSpan = serviceTrace.CreateSpan("My API", createOptions, clientContext); + EXPECT_FALSE(contextAndSpan.first.IsCancelled()); + } + + // Now let's verify what was logged via OpenTelemetry. + auto spans = m_spanData->GetSpans(); + EXPECT_EQ(1ul, spans.size()); + + VerifySpan(spans[0], R"( +{ + "name": "My API", + "kind": "internal", + "attributes": { + "az.namespace": "my-service", + "TestAttribute": 3 + }, + "library": { + "name": "my-service", + "version": "1.0beta-2" + } +})"); + } + + // Clear the global tracer provider set earlier in the test. + Azure::Core::Context::ApplicationContext.SetTracerProvider(nullptr); +} + TEST_F(OpenTelemetryServiceTests, NestSpans) { { @@ -635,7 +683,7 @@ TEST_F(OpenTelemetryServiceTests, ServiceApiImplementation) VerifySpan(spans[0], R"( { - "name": "HTTP GET #0", + "name": "HTTP GET", "kind": "client", "statusCode": "unset", "attributes": { diff --git a/sdk/core/azure-core/inc/azure/core/internal/tracing/service_tracing.hpp b/sdk/core/azure-core/inc/azure/core/internal/tracing/service_tracing.hpp index 9f9217c7d..d153161cf 100644 --- a/sdk/core/azure-core/inc/azure/core/internal/tracing/service_tracing.hpp +++ b/sdk/core/azure-core/inc/azure/core/internal/tracing/service_tracing.hpp @@ -180,12 +180,6 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal { // using TracingContext = std::pair, std::shared_ptr>; using TracingContext = std::shared_ptr; - static DiagnosticTracingFactory* DiagnosticFactoryFromContext( - Azure::Core::Context const& context); - - static Azure::Nullable TracingContextFromContext( - Azure::Core::Context const& context); - public: DiagnosticTracingFactory( Azure::Core::_internal::ClientOptions const& options, @@ -200,6 +194,7 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal { } DiagnosticTracingFactory() = default; + DiagnosticTracingFactory(DiagnosticTracingFactory const&) = default; /** @brief A ContextAndSpan provides an updated Context object and a new span object * which can be used to add events and attributes to the span. @@ -211,12 +206,15 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal { Azure::Core::Tracing::_internal::SpanKind const& spanKind, Azure::Core::Context const& clientContext); - static ContextAndSpan CreateSpanFromContext( + ContextAndSpan CreateSpan( std::string const& spanName, - Azure::Core::Tracing::_internal::SpanKind const& spanKind, + Azure::Core::Tracing::_internal::CreateSpanOptions& spanOptions, Azure::Core::Context const& clientContext); std::unique_ptr CreateAttributeSet(); + + static std::unique_ptr DiagnosticFactoryFromContext( + Azure::Core::Context const& context); }; /** diff --git a/sdk/core/azure-core/src/http/request_activity_policy.cpp b/sdk/core/azure-core/src/http/request_activity_policy.cpp index 556830a2e..504a4017e 100644 --- a/sdk/core/azure-core/src/http/request_activity_policy.cpp +++ b/sdk/core/azure-core/src/http/request_activity_policy.cpp @@ -23,68 +23,78 @@ std::unique_ptr RequestActivityPolicy::Send( NextHttpPolicy nextPolicy, Context const& context) const { - // Create a tracing span over the HTTP request. - std::stringstream ss; - // We know that the retry policy MUST be above us in the hierarchy, so ask it for the current - // retry count. - auto retryCount = RetryPolicy::GetRetryCount(context); - if (retryCount == -1) + // Find a tracing factory from our context. Note that the factory value is owned by the + // context chain so we can manage a raw pointer to the factory. + auto tracingFactory = DiagnosticTracingFactory::DiagnosticFactoryFromContext(context); + if (tracingFactory) { - // We don't have a RetryPolicy in the policy stack - just assume this is request 0. - retryCount = 0; - } - ss << "HTTP " << request.GetMethod().ToString() << " #" << retryCount; - auto contextAndSpan - = Azure::Core::Tracing::_internal::DiagnosticTracingFactory::CreateSpanFromContext( - ss.str(), SpanKind::Client, context); - auto scope = std::move(contextAndSpan.second); + // Create a tracing span over the HTTP request. + std::stringstream ss; + ss << "HTTP " << request.GetMethod().ToString(); - scope.AddAttribute(TracingAttributes::HttpMethod.ToString(), request.GetMethod().ToString()); - scope.AddAttribute("http.url", m_inputSanitizer.SanitizeUrl(request.GetUrl()).GetAbsoluteUrl()); - { + CreateSpanOptions createOptions; + createOptions.Kind = SpanKind::Client; + createOptions.Attributes = tracingFactory->CreateAttributeSet(); + // Note that the AttributeSet takes a *reference* to the values passed into the AttributeSet. + // This means that all the values passed into the AttributeSet MUST be stabilized across the + // lifetime of the AttributeSet. + std::string httpMethod = request.GetMethod().ToString(); + createOptions.Attributes->AddAttribute(TracingAttributes::HttpMethod.ToString(), httpMethod); + + std::string sanitizedUrl = m_inputSanitizer.SanitizeUrl(request.GetUrl()).GetAbsoluteUrl(); + createOptions.Attributes->AddAttribute("http.url", sanitizedUrl); Azure::Nullable requestId = request.GetHeader("x-ms-client-request-id"); if (requestId.HasValue()) { - scope.AddAttribute(TracingAttributes::RequestId.ToString(), requestId.Value()); + createOptions.Attributes->AddAttribute( + TracingAttributes::RequestId.ToString(), requestId.Value()); } - } - { auto userAgent = request.GetHeader("User-Agent"); if (userAgent.HasValue()) { - scope.AddAttribute(TracingAttributes::HttpUserAgent.ToString(), userAgent.Value()); + createOptions.Attributes->AddAttribute( + TracingAttributes::HttpUserAgent.ToString(), userAgent.Value()); } - } - // Propagate information from the scope to the HTTP headers. - // - // This will add the "traceparent" header and any other OpenTelemetry related headers. - scope.PropagateToHttpHeaders(request); + auto contextAndSpan = tracingFactory->CreateSpan(ss.str(), createOptions, context); + auto scope = std::move(contextAndSpan.second); - try - { - // Send the request on to the service. - auto response = nextPolicy.Send(request, contextAndSpan.first); + // Propagate information from the scope to the HTTP headers. + // + // This will add the "traceparent" header and any other OpenTelemetry related headers. + scope.PropagateToHttpHeaders(request); - // And register the headers we received from the service. - scope.AddAttribute( - TracingAttributes::HttpStatusCode.ToString(), - std::to_string(static_cast(response->GetStatusCode()))); - auto const& responseHeaders = response->GetHeaders(); - auto serviceRequestId = responseHeaders.find("x-ms-request-id"); - if (serviceRequestId != responseHeaders.end()) + try { - scope.AddAttribute(TracingAttributes::ServiceRequestId.ToString(), serviceRequestId->second); + // Send the request on to the service. + auto response = nextPolicy.Send(request, contextAndSpan.first); + + // And register the headers we received from the service. + scope.AddAttribute( + TracingAttributes::HttpStatusCode.ToString(), + std::to_string(static_cast(response->GetStatusCode()))); + auto const& responseHeaders = response->GetHeaders(); + auto serviceRequestId = responseHeaders.find("x-ms-request-id"); + if (serviceRequestId != responseHeaders.end()) + { + scope.AddAttribute( + TracingAttributes::ServiceRequestId.ToString(), serviceRequestId->second); + } + + return response; } + catch (const TransportException& e) + { + scope.AddEvent(e); + scope.SetStatus(SpanStatus::Error); - return response; + // Rethrow the exception. + throw; + } } - catch (const TransportException& e) + else { - scope.AddEvent(e); - - // Rethrow the exception. - throw; + return nextPolicy.Send(request, context); } } diff --git a/sdk/core/azure-core/src/tracing/tracing.cpp b/sdk/core/azure-core/src/tracing/tracing.cpp index 920c51c7d..0df489e30 100644 --- a/sdk/core/azure-core/src/tracing/tracing.cpp +++ b/sdk/core/azure-core/src/tracing/tracing.cpp @@ -26,7 +26,26 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal { Azure::Core::Tracing::_internal::SpanKind const& spanKind, Azure::Core::Context const& context) { - CreateSpanOptions createOptions; + if (m_serviceTracer) + { + Azure::Core::Context contextToUse = context; + CreateSpanOptions createOptions; + + createOptions.Kind = spanKind; + createOptions.Attributes = m_serviceTracer->CreateAttributeSet(); + return CreateSpan(methodName, createOptions, context); + } + else + { + return std::make_pair(context, ServiceSpan{}); + } + } + + DiagnosticTracingFactory::ContextAndSpan DiagnosticTracingFactory::CreateSpan( + std::string const& methodName, + Azure::Core::Tracing::_internal::CreateSpanOptions& createOptions, + Azure::Core::Context const& context) + { if (m_serviceTracer) { Azure::Core::Context contextToUse = context; @@ -50,12 +69,14 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal { // span if there is no parent span in the context createOptions.ParentSpan = nullptr; } - createOptions.Attributes = m_serviceTracer->CreateAttributeSet(); + + if (!createOptions.Attributes) + { + createOptions.Attributes = m_serviceTracer->CreateAttributeSet(); + } createOptions.Attributes->AddAttribute( TracingAttributes::AzNamespace.ToString(), m_serviceName); - createOptions.Kind = spanKind; - std::shared_ptr newSpan(m_serviceTracer->CreateSpan(methodName, createOptions)); TracingContext tracingContext = newSpan; Azure::Core::Context newContext = contextToUse.WithValue(ContextSpanKey, tracingContext); @@ -68,44 +89,14 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal { return std::make_pair(context, ServiceSpan{}); } } - DiagnosticTracingFactory::ContextAndSpan DiagnosticTracingFactory::CreateSpanFromContext( - std::string const& spanName, - Azure::Core::Tracing::_internal::SpanKind const& spanKind, - Azure::Core::Context const& context) - { - DiagnosticTracingFactory* tracingFactory - = DiagnosticTracingFactory::DiagnosticFactoryFromContext(context); - if (tracingFactory) - { - return tracingFactory->CreateSpan(spanName, spanKind, context); - } - else - { - return std::make_pair(context, ServiceSpan{}); - } - } - Azure::Nullable - DiagnosticTracingFactory::TracingContextFromContext(Azure::Core::Context const& context) - { - TracingContext traceContext; - if (context.TryGetValue(ContextSpanKey, traceContext)) - { - return traceContext; - } - else - { - return Azure::Nullable{}; - } - } - - DiagnosticTracingFactory* DiagnosticTracingFactory::DiagnosticFactoryFromContext( + std::unique_ptr DiagnosticTracingFactory::DiagnosticFactoryFromContext( Azure::Core::Context const& context) { DiagnosticTracingFactory* factory; if (context.TryGetValue(TracingFactoryContextKey, factory)) { - return factory; + return std::make_unique(*factory); } else { @@ -116,10 +107,13 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal { std::unique_ptr DiagnosticTracingFactory::CreateAttributeSet() { - return m_serviceTracer->CreateAttributeSet(); + if (m_serviceTracer) + { + return m_serviceTracer->CreateAttributeSet(); + } + return nullptr; } Azure::Core::Context::Key DiagnosticTracingFactory::ContextSpanKey; Azure::Core::Context::Key DiagnosticTracingFactory::TracingFactoryContextKey; - }}}} // namespace Azure::Core::Tracing::_internal diff --git a/sdk/core/azure-core/test/ut/request_activity_policy_test.cpp b/sdk/core/azure-core/test/ut/request_activity_policy_test.cpp index 6a001a1e4..2d5e34ad2 100644 --- a/sdk/core/azure-core/test/ut/request_activity_policy_test.cpp +++ b/sdk/core/azure-core/test/ut/request_activity_policy_test.cpp @@ -40,6 +40,31 @@ public: : HttpPolicy(), m_createResponse(createResponse){}; }; +class TestAttributeSet : public Azure::Core::Tracing::_internal::AttributeSet { + std::map m_attributes; + +public: + TestAttributeSet() : Azure::Core::Tracing::_internal::AttributeSet() {} + + // Inherited via AttributeSet + virtual void AddAttribute(std::string const&, bool) override {} + virtual void AddAttribute(std::string const&, int32_t) override {} + virtual void AddAttribute(std::string const&, int64_t) override {} + virtual void AddAttribute(std::string const&, uint64_t) override {} + virtual void AddAttribute(std::string const&, double) override {} + virtual void AddAttribute(std::string const& key, const char* val) override + { + m_attributes.emplace(std::make_pair(key, std::string(val))); + } + + virtual void AddAttribute(std::string const& key, std::string const& val) override + { + m_attributes.emplace(std::make_pair(key, val)); + } + + std::map const& GetAttributes() { return m_attributes; } +}; + // Dummy service tracing class. class TestSpan final : public Azure::Core::Tracing::_internal::Span { std::vector m_events; @@ -47,9 +72,18 @@ class TestSpan final : public Azure::Core::Tracing::_internal::Span { std::string m_spanName; public: - TestSpan(std::string const& spanName) + TestSpan(std::string const& spanName, CreateSpanOptions const& options) : Azure::Core::Tracing::_internal::Span(), m_spanName(spanName) { + if (options.Attributes) + { + TestAttributeSet* testAttributes = static_cast(options.Attributes.get()); + + for (auto const& attribute : testAttributes->GetAttributes()) + { + m_stringAttributes.emplace(attribute); + } + } } // Inherited via Span @@ -78,29 +112,15 @@ public: std::map const& GetAttributes() { return m_stringAttributes; } }; -class TestAttributeSet : public Azure::Core::Tracing::_internal::AttributeSet { -public: - TestAttributeSet() : Azure::Core::Tracing::_internal::AttributeSet() {} - - // Inherited via AttributeSet - virtual void AddAttribute(std::string const&, bool) override {} - virtual void AddAttribute(std::string const&, int32_t) override {} - virtual void AddAttribute(std::string const&, int64_t) override {} - virtual void AddAttribute(std::string const&, uint64_t) override {} - virtual void AddAttribute(std::string const&, double) override {} - virtual void AddAttribute(std::string const&, const char*) override {} - virtual void AddAttribute(std::string const&, std::string const&) override {} -}; - class TestTracer final : public Azure::Core::Tracing::_internal::Tracer { mutable std::vector> m_spans; public: TestTracer(std::string const&, std::string const&) : Azure::Core::Tracing::_internal::Tracer() {} - std::shared_ptr CreateSpan(std::string const& spanName, CreateSpanOptions const&) + std::shared_ptr CreateSpan(std::string const& spanName, CreateSpanOptions const& options) const override { - auto returnSpan(std::make_shared(spanName)); + auto returnSpan(std::make_shared(spanName, options)); m_spans.push_back(returnSpan); return returnSpan; } @@ -162,7 +182,7 @@ TEST(RequestActivityPolicy, Basic) auto& tracer = testTracer->GetTracers().front(); EXPECT_EQ(2ul, tracer->GetSpans().size()); EXPECT_EQ("My API", tracer->GetSpans()[0]->GetName()); - EXPECT_EQ("HTTP GET #0", tracer->GetSpans()[1]->GetName()); + EXPECT_EQ("HTTP GET", tracer->GetSpans()[1]->GetName()); EXPECT_EQ("GET", tracer->GetSpans()[1]->GetAttributes().at("http.method")); } @@ -198,7 +218,7 @@ TEST(RequestActivityPolicy, Basic) auto& tracer = testTracer->GetTracers().front(); EXPECT_EQ(2ul, tracer->GetSpans().size()); EXPECT_EQ("My API", tracer->GetSpans()[0]->GetName()); - EXPECT_EQ("HTTP GET #0", tracer->GetSpans()[1]->GetName()); + EXPECT_EQ("HTTP GET", tracer->GetSpans()[1]->GetName()); EXPECT_EQ("GET", tracer->GetSpans()[1]->GetAttributes().at("http.method")); } } @@ -253,9 +273,9 @@ TEST(RequestActivityPolicy, TryRetries) auto& tracer = testTracer->GetTracers().front(); EXPECT_EQ(4ul, tracer->GetSpans().size()); EXPECT_EQ("My API", tracer->GetSpans()[0]->GetName()); - EXPECT_EQ("HTTP GET #0", tracer->GetSpans()[1]->GetName()); - EXPECT_EQ("HTTP GET #1", tracer->GetSpans()[2]->GetName()); - EXPECT_EQ("HTTP GET #2", tracer->GetSpans()[3]->GetName()); + EXPECT_EQ("HTTP GET", tracer->GetSpans()[1]->GetName()); + EXPECT_EQ("HTTP GET", tracer->GetSpans()[2]->GetName()); + EXPECT_EQ("HTTP GET", tracer->GetSpans()[3]->GetName()); EXPECT_EQ("GET", tracer->GetSpans()[1]->GetAttributes().at("http.method")); EXPECT_EQ("408", tracer->GetSpans()[1]->GetAttributes().at("http.status_code")); EXPECT_EQ("408", tracer->GetSpans()[2]->GetAttributes().at("http.status_code")); diff --git a/sdk/core/azure-core/test/ut/service_tracing_test.cpp b/sdk/core/azure-core/test/ut/service_tracing_test.cpp index 7675a6236..4c3988233 100644 --- a/sdk/core/azure-core/test/ut/service_tracing_test.cpp +++ b/sdk/core/azure-core/test/ut/service_tracing_test.cpp @@ -151,4 +151,29 @@ TEST(DiagnosticTracingFactory, BasicServiceSpanTests) span.AddAttributes(*attributeSet); span.SetStatus(SpanStatus::Error); } + + // Now run all the previous tests on a DiagnosticTracingFactory created *without* a tracing + // provider. + { + Azure::Core::_internal::ClientOptions clientOptions; + Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace( + clientOptions, "my-service-cpp", "1.0b2"); + + auto contextAndSpan = serviceTrace.CreateSpan( + "My API", Azure::Core::Tracing::_internal::SpanKind::Internal, {}); + ServiceSpan span = std::move(contextAndSpan.second); + + span.End(); + span.AddEvent("New Event"); + span.AddEvent(std::runtime_error("Exception")); + std::unique_ptr attributeSet + = serviceTrace.CreateAttributeSet(); + if (attributeSet) + { + attributeSet->AddAttribute("Joe", "Joe'sValue"); + span.AddEvent("AttributeEvent", *attributeSet); + span.AddAttributes(*attributeSet); + } + span.SetStatus(SpanStatus::Error); + } }