Additional OpenTelemetry Feedback...

doc/DistributedTracing.md

@@ -133,7 +133,7 @@ There are two steps needed to integrate Distributed Tracing with a Service Clien
 To add a new `DiagnosticTracingFactory` to the client, simply add the class as a member:
 
 ```c++
-  Azure::Core::Tracing::_internal::DiagnosticTracingFactory m_tracingFactory;
+  Azure::Core::Tracing::_internal::TracingContextFactory m_tracingFactory;
 
 ```
 
@@ -161,8 +161,8 @@ And construct the new tracing factory in the service constructor:
     auto contextAndSpan = m_tracingFactory.CreateSpan(
         "ServiceMethod", Azure::Core::Tracing::_internal::SpanKind::Internal, context);
 
-    // contextAndSpan.first is the new context for the operation.
-    // contextAndSpan.second is the new span for the operation.
+    // contextAndSpan.Context is the new context for the operation.
+    // contextAndSpan.Span is the new span for the operation.
 
     try
     {
@@ -171,15 +171,15 @@ And construct the new tracing factory in the service constructor:
           HttpMethod::Get, Azure::Core::Url("<Service URL>"));
 
       std::unique_ptr<Azure::Core::Http::RawResponse> response
-          = m_pipeline->Send(requestToSend, contextAndSpan.first);
-      contextAndSpan.second.SetStatus(Azure::Core::Tracing::_internal::SpanStatus::Ok);
+          = m_pipeline->Send(requestToSend, contextAndSpan.Context);
+      contextAndSpan.Span.SetStatus(Azure::Core::Tracing::_internal::SpanStatus::Ok);
       return Azure::Response<std::string>("", std::move(response));
     }
     catch (std::exception const& ex)
     {
       // Register that the exception has happened and that the span is now in error.
-      contextAndSpan.second.AddEvent(ex);
-      contextAndSpan.second.SetStatus(Azure::Core::Tracing::_internal::SpanStatus::Error);
+      contextAndSpan.Span.AddEvent(ex);
+      contextAndSpan.Span.SetStatus(Azure::Core::Tracing::_internal::SpanStatus::Error);
       throw;
     }
 

sdk/core/azure-core-tracing-opentelemetry/test/ut/service_support_test.cpp

@@ -274,22 +274,21 @@ class OpenTelemetryServiceTests : public Azure::Core::Test::TestBase {
 TEST_F(OpenTelemetryServiceTests, SimplestTest)
 {
   {
-    Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace;
+    Azure::Core::Tracing::_internal::TracingContextFactory serviceTrace;
   }
   {
     Azure::Core::_internal::ClientOptions clientOptions;
-    Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
+    Azure::Core::Tracing::_internal::TracingContextFactory serviceTrace(
         clientOptions, "my-service-cpp", "1.0b2");
   }
 
   {
     Azure::Core::_internal::ClientOptions clientOptions;
-    Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
+    Azure::Core::Tracing::_internal::TracingContextFactory serviceTrace(
         clientOptions, "my-service-cpp", "1.0b2");
 
-    auto contextAndSpan = serviceTrace.CreateSpan(
-        "My API", Azure::Core::Tracing::_internal::SpanKind::Internal, {});
-    EXPECT_FALSE(contextAndSpan.first.IsCancelled());
+    auto contextAndSpan = serviceTrace.CreateTracingContext("My API", {});
+    EXPECT_FALSE(contextAndSpan.Context.IsCancelled());
   }
 }
 
@@ -320,13 +319,12 @@ TEST_F(OpenTelemetryServiceTests, CreateWithExplicitProvider)
       clientOptions.Telemetry.TracingProvider = provider;
       clientOptions.Telemetry.ApplicationId = "MyApplication";
 
-      Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
+      Azure::Core::Tracing::_internal::TracingContextFactory serviceTrace(
           clientOptions, "my-service", "1.0beta-2");
 
       Azure::Core::Context clientContext;
-      auto contextAndSpan = serviceTrace.CreateSpan(
-          "My API", Azure::Core::Tracing::_internal::SpanKind::Internal, clientContext);
-      EXPECT_FALSE(contextAndSpan.first.IsCancelled());
+      auto contextAndSpan = serviceTrace.CreateTracingContext("My API", clientContext);
+      EXPECT_FALSE(contextAndSpan.Context.IsCancelled());
     }
     // Now let's verify what was logged via OpenTelemetry.
     auto spans = m_spanData->GetSpans();
@@ -360,13 +358,12 @@ TEST_F(OpenTelemetryServiceTests, CreateWithImplicitProvider)
       Azure::Core::_internal::ClientOptions clientOptions;
       clientOptions.Telemetry.ApplicationId = "MyApplication";
 
-      Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
+      Azure::Core::Tracing::_internal::TracingContextFactory serviceTrace(
           clientOptions, "my-service", "1.0beta-2");
 
       Azure::Core::Context clientContext;
-      auto contextAndSpan = serviceTrace.CreateSpan(
-          "My API", Azure::Core::Tracing::_internal::SpanKind::Internal, clientContext);
-      EXPECT_FALSE(contextAndSpan.first.IsCancelled());
+      auto contextAndSpan = serviceTrace.CreateTracingContext("My API", clientContext);
+      EXPECT_FALSE(contextAndSpan.Context.IsCancelled());
     }
 
     // Now let's verify what was logged via OpenTelemetry.
@@ -404,16 +401,17 @@ TEST_F(OpenTelemetryServiceTests, CreateSpanWithOptions)
       Azure::Core::_internal::ClientOptions clientOptions;
       clientOptions.Telemetry.ApplicationId = "MyApplication";
 
-      Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
+      Azure::Core::Tracing::_internal::TracingContextFactory 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());
+      auto contextAndSpan
+          = serviceTrace.CreateTracingContext("My API", createOptions, clientContext);
+      EXPECT_FALSE(contextAndSpan.Context.IsCancelled());
     }
 
     // Now let's verify what was logged via OpenTelemetry.
@@ -456,21 +454,25 @@ TEST_F(OpenTelemetryServiceTests, NestSpans)
       Azure::Core::_internal::ClientOptions clientOptions;
       clientOptions.Telemetry.ApplicationId = "MyApplication";
 
-      Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
+      Azure::Core::Tracing::_internal::TracingContextFactory serviceTrace(
           clientOptions, "my-service", "1.0beta-2");
 
       Azure::Core::Context parentContext;
-      auto contextAndSpan = serviceTrace.CreateSpan(
-          "My API", Azure::Core::Tracing::_internal::SpanKind::Client, parentContext);
-      EXPECT_FALSE(contextAndSpan.first.IsCancelled());
-      parentContext = contextAndSpan.first;
-      contextAndSpan.second.PropagateToHttpHeaders(outerRequest);
+      Azure::Core::Tracing::_internal::CreateSpanOptions createOptions;
+      createOptions.Kind = Azure::Core::Tracing::_internal::SpanKind::Client;
+      auto contextAndSpan
+          = serviceTrace.CreateTracingContext("My API", createOptions, parentContext);
+      EXPECT_FALSE(contextAndSpan.Context.IsCancelled());
+      parentContext = contextAndSpan.Context;
+      contextAndSpan.Span.PropagateToHttpHeaders(outerRequest);
 
       {
-        auto innerContextAndSpan = serviceTrace.CreateSpan(
-            "Nested API", Azure::Core::Tracing::_internal::SpanKind::Server, parentContext);
-        EXPECT_FALSE(innerContextAndSpan.first.IsCancelled());
-        innerContextAndSpan.second.PropagateToHttpHeaders(innerRequest);
+        Azure::Core::Tracing::_internal::CreateSpanOptions innerOptions;
+        innerOptions.Kind = Azure::Core::Tracing::_internal::SpanKind::Server;
+        auto innerContextAndSpan
+            = serviceTrace.CreateTracingContext("Nested API", innerOptions, parentContext);
+        EXPECT_FALSE(innerContextAndSpan.Context.IsCancelled());
+        innerContextAndSpan.Span.PropagateToHttpHeaders(innerRequest);
       }
     }
     // Now let's verify what was logged via OpenTelemetry.
@@ -580,7 +582,7 @@ class ServiceClientOptions : public Azure::Core::_internal::ClientOptions {
 class ServiceClient {
 private:
   ServiceClientOptions m_clientOptions;
-  Azure::Core::Tracing::_internal::DiagnosticTracingFactory m_tracingFactory;
+  Azure::Core::Tracing::_internal::TracingContextFactory m_tracingFactory;
   std::unique_ptr<Azure::Core::Http::_internal::HttpPipeline> m_pipeline;
 
 public:
@@ -612,31 +614,29 @@ class ServiceClient {
 
   Azure::Response<std::string> GetConfigurationString(
       std::string const& inputString,
-      Azure::Core::Context const& context = Azure::Core::Context{})
+      Azure::Core::Context const& context = Azure::Core::Context{}) const
   {
-    auto contextAndSpan = m_tracingFactory.CreateSpan(
-        "GetConfigurationString", Azure::Core::Tracing::_internal::SpanKind::Internal, context);
+    auto contextAndSpan = m_tracingFactory.CreateTracingContext("GetConfigurationString", context);
 
     // <Call Into Service via an HTTP pipeline>
     Azure::Core::Http::Request requestToSend(
         HttpMethod::Get, Azure::Core::Url("https://www.microsoft.com/"));
 
     std::unique_ptr<Azure::Core::Http::RawResponse> response
-        = m_pipeline->Send(requestToSend, contextAndSpan.first);
+        = m_pipeline->Send(requestToSend, contextAndSpan.Context);
 
     // Reflect that the operation was successful.
-    contextAndSpan.second.SetStatus(Azure::Core::Tracing::_internal::SpanStatus::Ok);
+    contextAndSpan.Span.SetStatus(Azure::Core::Tracing::_internal::SpanStatus::Ok);
     Azure::Response<std::string> rv(inputString, std::move(response));
     return rv;
     // When contextAndSpan.second goes out of scope, it ends the span, which will record it.
   }
 
   Azure::Response<std::string> ApiWhichThrows(
       std::string const&,
-      Azure::Core::Context const& context = Azure::Core::Context{})
+      Azure::Core::Context const& context = Azure::Core::Context{}) const
   {
-    auto contextAndSpan = m_tracingFactory.CreateSpan(
-        "ApiWhichThrows", Azure::Core::Tracing::_internal::SpanKind::Internal, context);
+    auto contextAndSpan = m_tracingFactory.CreateTracingContext("ApiWhichThrows", context);
 
     try
     {
@@ -645,14 +645,14 @@ class ServiceClient {
           HttpMethod::Get, Azure::Core::Url("https://www.microsoft.com/:12345/index.html"));
 
       std::unique_ptr<Azure::Core::Http::RawResponse> response
-          = m_pipeline->Send(requestToSend, contextAndSpan.first);
+          = m_pipeline->Send(requestToSend, contextAndSpan.Context);
       return Azure::Response<std::string>("", std::move(response));
     }
     catch (std::exception const& ex)
     {
       // Register that the exception has happened and that the span is now in error.
-      contextAndSpan.second.AddEvent(ex);
-      contextAndSpan.second.SetStatus(Azure::Core::Tracing::_internal::SpanStatus::Error);
+      contextAndSpan.Span.AddEvent(ex);
+      contextAndSpan.Span.SetStatus(Azure::Core::Tracing::_internal::SpanStatus::Error);
       throw;
     }
 

sdk/core/azure-core/inc/azure/core/internal/tracing/service_tracing.hpp

@@ -29,7 +29,7 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
   private:
     std::shared_ptr<Span> m_span;
 
-    friend class DiagnosticTracingFactory;
+    friend class TracingContextFactory;
     ServiceSpan() = default;
     explicit ServiceSpan(std::shared_ptr<Span> span) : m_span(span) {}
 
@@ -49,11 +49,11 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
       }
     }
 
-    void End(Azure::Nullable<Azure::DateTime> = Azure::Nullable<Azure::DateTime>{}) override
+    void End(Azure::Nullable<Azure::DateTime> endTime = Azure::Nullable<Azure::DateTime>{}) override
     {
       if (m_span)
       {
-        m_span->End();
+        m_span->End(endTime);
       }
     }
     void SetStatus(
@@ -161,7 +161,7 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
    * @details Each service implementation SHOULD have a member variable which aids in managing
    * the distributed tracing for the service.
    */
-  class DiagnosticTracingFactory final {
+  class TracingContextFactory final {
   private:
     std::string m_serviceName;
     std::string m_serviceVersion;
@@ -177,11 +177,9 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
      */
     static Azure::Core::Context::Key ContextSpanKey;
     static Azure::Core::Context::Key TracingFactoryContextKey;
-    //    using TracingContext = std::pair<std::shared_ptr<Span>, std::shared_ptr<Tracer>>;
-    using TracingContext = std::shared_ptr<Span>;
 
   public:
-    DiagnosticTracingFactory(
+    TracingContextFactory(
         Azure::Core::_internal::ClientOptions const& options,
         std::string serviceName,
         std::string serviceVersion)
@@ -193,27 +191,59 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
     {
     }
 
-    DiagnosticTracingFactory() = default;
-    DiagnosticTracingFactory(DiagnosticTracingFactory const&) = default;
+    TracingContextFactory() = default;
+    TracingContextFactory(TracingContextFactory const&) = default;
 
-    /** @brief A ContextAndSpan provides an updated Context object and a new span object
+    /** @brief A TracingContext provides an updated Context object and a new span object
      * which can be used to add events and attributes to the span.
      */
-    using ContextAndSpan = std::pair<Azure::Core::Context, ServiceSpan>;
+    struct TracingContext
+    {
+      /**
+       * @brief New Context to be used for subsequent methods which take a Context parameter.
+       */
+      Azure::Core::Context Context;
+      /**
+       * @brief Distributed Tracing Span which can be used to update status if the API succeeds or
+       * fails.
+       */
+      ServiceSpan Span;
+    };
 
-    ContextAndSpan CreateSpan(
+    /**
+     * @brief Create a span with the specified span name.
+     *
+     * @details This method is a convenience method intended for use by service clients, it creates
+     * a SpanKind::Internal span and context.
+     *
+     * @param spanName Name for the span to be created.
+     * @param context parent context object for the newly created span.
+     *
+     * @returns Newly allocated context and Span object.
+     *
+     */
+    TracingContext CreateTracingContext(
         std::string const& spanName,
-        Azure::Core::Tracing::_internal::SpanKind const& spanKind,
-        Azure::Core::Context const& clientContext);
+        Azure::Core::Context const& context) const;
 
-    ContextAndSpan CreateSpan(
+    /**
+     * @brief Create a span with the specified span name and create options.
+     *
+     * @param spanName Name for the span to be created.
+     * @param spanOptions Options for the newly created span.
+     * @param context parent context object for the newly created span.
+     *
+     * @returns Newly allocated context and Span object.
+     *
+     */
+    TracingContext CreateTracingContext(
         std::string const& spanName,
         Azure::Core::Tracing::_internal::CreateSpanOptions& spanOptions,
-        Azure::Core::Context const& clientContext);
+        Azure::Core::Context const& context) const;
 
-    std::unique_ptr<Azure::Core::Tracing::_internal::AttributeSet> CreateAttributeSet();
+    std::unique_ptr<Azure::Core::Tracing::_internal::AttributeSet> CreateAttributeSet() const;
 
-    static std::unique_ptr<DiagnosticTracingFactory> DiagnosticFactoryFromContext(
+    static std::unique_ptr<TracingContextFactory> CreateFromContext(
         Azure::Core::Context const& context);
   };
 

sdk/core/azure-core/src/http/request_activity_policy.cpp

@@ -25,7 +25,7 @@ std::unique_ptr<RawResponse> RequestActivityPolicy::Send(
 {
   // 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);
+  auto tracingFactory = TracingContextFactory::CreateFromContext(context);
   if (tracingFactory)
   {
     // Create a tracing span over the HTTP request.
@@ -38,27 +38,32 @@ std::unique_ptr<RawResponse> RequestActivityPolicy::Send(
     // 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();
+    // Note that request.GetMethod() returns an HttpMethod object, which is always a static
+    // object, and thus its lifetime is constant. That is not the case for the other values
+    // stored in the attributes.
+    createOptions.Attributes->AddAttribute(
+        TracingAttributes::HttpMethod.ToString(), request.GetMethod().ToString());
+
+    const std::string sanitizedUrl
+        = m_inputSanitizer.SanitizeUrl(request.GetUrl()).GetAbsoluteUrl();
     createOptions.Attributes->AddAttribute("http.url", sanitizedUrl);
-    Azure::Nullable<std::string> requestId = request.GetHeader("x-ms-client-request-id");
+    const Azure::Nullable<std::string> requestId = request.GetHeader("x-ms-client-request-id");
     if (requestId.HasValue())
     {
       createOptions.Attributes->AddAttribute(
           TracingAttributes::RequestId.ToString(), requestId.Value());
     }
 
-    auto userAgent = request.GetHeader("User-Agent");
+    const auto userAgent = request.GetHeader("User-Agent");
     if (userAgent.HasValue())
     {
       createOptions.Attributes->AddAttribute(
           TracingAttributes::HttpUserAgent.ToString(), userAgent.Value());
     }
 
-    auto contextAndSpan = tracingFactory->CreateSpan(ss.str(), createOptions, context);
-    auto scope = std::move(contextAndSpan.second);
+    auto contextAndSpan = tracingFactory->CreateTracingContext(ss.str(), createOptions, context);
+    auto scope = std::move(contextAndSpan.Span);
 
     // Propagate information from the scope to the HTTP headers.
     //
@@ -68,7 +73,7 @@ std::unique_ptr<RawResponse> RequestActivityPolicy::Send(
     try
     {
       // Send the request on to the service.
-      auto response = nextPolicy.Send(request, contextAndSpan.first);
+      auto response = nextPolicy.Send(request, contextAndSpan.Context);
 
       // And register the headers we received from the service.
       scope.AddAttribute(