Skip to content

Additional OpenTelemetry Feedback... #3691

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
LarryOsterman merged 14 commits into from
Jun 3, 2022
Merged

Additional OpenTelemetry Feedback... #3691

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6512cb5
OpenTelemetry API Review Feedback
LarryOsterman Jun 1, 2022
3e706d8
clang-format
LarryOsterman Jun 1, 2022
83ffed9
Returns std::unique_ptr<DiagnosticTracingFactory instead of raw pointer
LarryOsterman Jun 1, 2022
e8c474e
clang_format
LarryOsterman Jun 1, 2022
91afd74
Late breaking pull request feedback
LarryOsterman Jun 2, 2022
29d62b8
Merge branch 'main' into larryo/opentelemetryfeedback
LarryOsterman Jun 2, 2022
e82daf5
Merge issues
LarryOsterman Jun 2, 2022
aa40e69
Merge conflicts
LarryOsterman Jun 2, 2022
37c65c3
clang_format
LarryOsterman Jun 2, 2022
8ab1ef7
Renamed clientContext parameter to CreateSpan
LarryOsterman Jun 2, 2022
aa43678
Renamed ContextAndSpanFactory to TracingContextFactory
LarryOsterman Jun 2, 2022
78770f5
clang_format
LarryOsterman Jun 2, 2022
922f86f
TracingContextFactory::FromContext->TracingContextFactory::CreateFrom…
LarryOsterman Jun 2, 2022
a0d3dce
Pass endTime to span->End
LarryOsterman Jun 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 22 additions & 24 deletions sdk/core/azure-core-tracing-opentelemetry/test/ut/service_support_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,21 +274,20 @@ class OpenTelemetryServiceTests : public Azure::Core::Test::TestBase {
TEST_F(OpenTelemetryServiceTests, SimplestTest)
{
{
Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace;
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace;
}
{
Azure::Core::_internal::ClientOptions clientOptions;
Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace(
clientOptions, "my-service-cpp", "1.0b2");
}

{
Azure::Core::_internal::ClientOptions clientOptions;
Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace(
clientOptions, "my-service-cpp", "1.0b2");

auto contextAndSpan = serviceTrace.CreateSpan(
"My API", Azure::Core::Tracing::_internal::SpanKind::Internal, {});
auto contextAndSpan = serviceTrace.CreateSpan("My API", {});
EXPECT_FALSE(contextAndSpan.first.IsCancelled());
}
}
Expand Down Expand Up @@ -320,12 +319,11 @@ TEST_F(OpenTelemetryServiceTests, CreateWithExplicitProvider)
clientOptions.Telemetry.TracingProvider = provider;
clientOptions.Telemetry.ApplicationId = "MyApplication";

Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace(
clientOptions, "my-service", "1.0beta-2");

Azure::Core::Context clientContext;
auto contextAndSpan = serviceTrace.CreateSpan(
"My API", Azure::Core::Tracing::_internal::SpanKind::Internal, clientContext);
auto contextAndSpan = serviceTrace.CreateSpan("My API", clientContext);
EXPECT_FALSE(contextAndSpan.first.IsCancelled());
}
// Now let's verify what was logged via OpenTelemetry.
Expand Down Expand Up @@ -360,12 +358,11 @@ TEST_F(OpenTelemetryServiceTests, CreateWithImplicitProvider)
Azure::Core::_internal::ClientOptions clientOptions;
clientOptions.Telemetry.ApplicationId = "MyApplication";

Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace(
clientOptions, "my-service", "1.0beta-2");

Azure::Core::Context clientContext;
auto contextAndSpan = serviceTrace.CreateSpan(
"My API", Azure::Core::Tracing::_internal::SpanKind::Internal, clientContext);
auto contextAndSpan = serviceTrace.CreateSpan("My API", clientContext);
EXPECT_FALSE(contextAndSpan.first.IsCancelled());
}

Expand Down Expand Up @@ -404,7 +401,7 @@ TEST_F(OpenTelemetryServiceTests, CreateSpanWithOptions)
Azure::Core::_internal::ClientOptions clientOptions;
clientOptions.Telemetry.ApplicationId = "MyApplication";

Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace(
clientOptions, "my-service", "1.0beta-2");

Azure::Core::Context clientContext;
Expand Down Expand Up @@ -456,19 +453,22 @@ TEST_F(OpenTelemetryServiceTests, NestSpans)
Azure::Core::_internal::ClientOptions clientOptions;
clientOptions.Telemetry.ApplicationId = "MyApplication";

Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace(
clientOptions, "my-service", "1.0beta-2");

Azure::Core::Context parentContext;
auto contextAndSpan = serviceTrace.CreateSpan(
"My API", Azure::Core::Tracing::_internal::SpanKind::Client, parentContext);
Azure::Core::Tracing::_internal::CreateSpanOptions createOptions;
createOptions.Kind = Azure::Core::Tracing::_internal::SpanKind::Client;
auto contextAndSpan = serviceTrace.CreateSpan("My API", createOptions, parentContext);
EXPECT_FALSE(contextAndSpan.first.IsCancelled());
parentContext = contextAndSpan.first;
contextAndSpan.second.PropagateToHttpHeaders(outerRequest);

{
auto innerContextAndSpan = serviceTrace.CreateSpan(
"Nested API", Azure::Core::Tracing::_internal::SpanKind::Server, parentContext);
Azure::Core::Tracing::_internal::CreateSpanOptions innerOptions;
innerOptions.Kind = Azure::Core::Tracing::_internal::SpanKind::Server;
auto innerContextAndSpan
= serviceTrace.CreateSpan("Nested API", innerOptions, parentContext);
EXPECT_FALSE(innerContextAndSpan.first.IsCancelled());
innerContextAndSpan.second.PropagateToHttpHeaders(innerRequest);
}
Expand Down Expand Up @@ -580,7 +580,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::ContextAndSpanFactory m_tracingFactory;
std::unique_ptr<Azure::Core::Http::_internal::HttpPipeline> m_pipeline;

public:
Expand Down Expand Up @@ -612,10 +612,9 @@ 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.CreateSpan("GetConfigurationString", context);

// <Call Into Service via an HTTP pipeline>
Azure::Core::Http::Request requestToSend(
Expand All @@ -633,10 +632,9 @@ class ServiceClient {

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.CreateSpan("ApiWhichThrows", context);

try
{
Expand Down
41 changes: 31 additions & 10 deletions sdk/core/azure-core/inc/azure/core/internal/tracing/service_tracing.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
private:
std::shared_ptr<Span> m_span;

friend class DiagnosticTracingFactory;
friend class ContextAndSpanFactory;
Comment thread
LarryOsterman marked this conversation as resolved.
Outdated
ServiceSpan() = default;
explicit ServiceSpan(std::shared_ptr<Span> span) : m_span(span) {}

Expand Down Expand Up @@ -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 ContextAndSpanFactory final {
private:
std::string m_serviceName;
std::string m_serviceVersion;
Expand All @@ -181,7 +181,7 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
using TracingContext = std::shared_ptr<Span>;

public:
DiagnosticTracingFactory(
ContextAndSpanFactory(
Azure::Core::_internal::ClientOptions const& options,
std::string serviceName,
std::string serviceVersion)
Expand All @@ -193,27 +193,48 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
{
}

DiagnosticTracingFactory() = default;
DiagnosticTracingFactory(DiagnosticTracingFactory const&) = default;
ContextAndSpanFactory() = default;
ContextAndSpanFactory(ContextAndSpanFactory 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.
*/
using ContextAndSpan = std::pair<Azure::Core::Context, ServiceSpan>;

/**
* @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 clientContext parent context object.
*
* @returns Newly allocated context and Span object.
Comment thread
LarryOsterman marked this conversation as resolved.
*
*/
ContextAndSpan CreateSpan(
std::string const& spanName,
Azure::Core::Tracing::_internal::SpanKind const& spanKind,
Azure::Core::Context const& clientContext);
Azure::Core::Context const& clientContext) const;
Comment thread
LarryOsterman marked this conversation as resolved.
Outdated

/**
* @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 clientContext parent context object.
*
* @returns Newly allocated context and Span object.
*
*/
ContextAndSpan CreateSpan(
std::string const& spanName,
Azure::Core::Tracing::_internal::CreateSpanOptions& spanOptions,
Azure::Core::Context const& clientContext);
Azure::Core::Context const& clientContext) 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<ContextAndSpanFactory> ContextAndSpanFactoryFromContext(
Comment thread
LarryOsterman marked this conversation as resolved.
Outdated
Azure::Core::Context const& context);
};

Expand Down
16 changes: 10 additions & 6 deletions sdk/core/azure-core/src/http/request_activity_policy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ContextAndSpanFactory::ContextAndSpanFactoryFromContext(context);
if (tracingFactory)
{
// Create a tracing span over the HTTP request.
Expand All @@ -38,19 +38,23 @@ 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(
Expand Down
25 changes: 12 additions & 13 deletions sdk/core/azure-core/src/tracing/tracing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,16 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
const TracingAttributes TracingAttributes::RequestId("requestId");
const TracingAttributes TracingAttributes::HttpStatusCode("http.status_code");

DiagnosticTracingFactory::ContextAndSpan DiagnosticTracingFactory::CreateSpan(
ContextAndSpanFactory::ContextAndSpan ContextAndSpanFactory::CreateSpan(
std::string const& methodName,
Azure::Core::Tracing::_internal::SpanKind const& spanKind,
Azure::Core::Context const& context)
Azure::Core::Context const& context) const
{
if (m_serviceTracer)
{
Azure::Core::Context contextToUse = context;
CreateSpanOptions createOptions;

createOptions.Kind = spanKind;
createOptions.Kind = SpanKind::Internal;
createOptions.Attributes = m_serviceTracer->CreateAttributeSet();
return CreateSpan(methodName, createOptions, context);
}
Expand All @@ -41,17 +40,17 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
}
}

DiagnosticTracingFactory::ContextAndSpan DiagnosticTracingFactory::CreateSpan(
ContextAndSpanFactory::ContextAndSpan ContextAndSpanFactory::CreateSpan(
std::string const& methodName,
Azure::Core::Tracing::_internal::CreateSpanOptions& createOptions,
Azure::Core::Context const& context)
Azure::Core::Context const& context) const
{
if (m_serviceTracer)
{
Azure::Core::Context contextToUse = context;

// Ensure that the factory is available in the context chain.
DiagnosticTracingFactory* tracingFactoryFromContext;
ContextAndSpanFactory const* tracingFactoryFromContext;
if (!context.TryGetValue(TracingFactoryContextKey, tracingFactoryFromContext))
{
contextToUse = context.WithValue(TracingFactoryContextKey, this);
Expand Down Expand Up @@ -90,13 +89,13 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
}
}

std::unique_ptr<DiagnosticTracingFactory> DiagnosticTracingFactory::DiagnosticFactoryFromContext(
std::unique_ptr<ContextAndSpanFactory> ContextAndSpanFactory::ContextAndSpanFactoryFromContext(
Azure::Core::Context const& context)
{
DiagnosticTracingFactory* factory;
ContextAndSpanFactory const* factory;
if (context.TryGetValue(TracingFactoryContextKey, factory))
{
return std::make_unique<DiagnosticTracingFactory>(*factory);
return std::make_unique<ContextAndSpanFactory>(*factory);
}
else
{
Expand All @@ -105,7 +104,7 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
}

std::unique_ptr<Azure::Core::Tracing::_internal::AttributeSet>
DiagnosticTracingFactory::CreateAttributeSet()
ContextAndSpanFactory::CreateAttributeSet() const
{
if (m_serviceTracer)
{
Expand All @@ -114,6 +113,6 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
return nullptr;
}

Azure::Core::Context::Key DiagnosticTracingFactory::ContextSpanKey;
Azure::Core::Context::Key DiagnosticTracingFactory::TracingFactoryContextKey;
Azure::Core::Context::Key ContextAndSpanFactory::ContextSpanKey;
Azure::Core::Context::Key ContextAndSpanFactory::TracingFactoryContextKey;
}}}} // namespace Azure::Core::Tracing::_internal
15 changes: 6 additions & 9 deletions sdk/core/azure-core/test/ut/request_activity_policy_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,10 @@ TEST(RequestActivityPolicy, Basic)

Azure::Core::_internal::ClientOptions clientOptions;
clientOptions.Telemetry.TracingProvider = testTracer;
Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace(
clientOptions, "my-service-cpp", "1.0b2");

auto contextAndSpan = serviceTrace.CreateSpan(
"My API", Azure::Core::Tracing::_internal::SpanKind::Internal, {});
auto contextAndSpan = serviceTrace.CreateSpan("My API", {});
Azure::Core::Context callContext = std::move(contextAndSpan.first);
Request request(HttpMethod::Get, Url("https://www.microsoft.com"));

Expand Down Expand Up @@ -192,10 +191,9 @@ TEST(RequestActivityPolicy, Basic)

Azure::Core::_internal::ClientOptions clientOptions;
clientOptions.Telemetry.TracingProvider = testTracer;
Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace(
clientOptions, "my-service-cpp", "1.0b2");
auto contextAndSpan = serviceTrace.CreateSpan(
"My API", Azure::Core::Tracing::_internal::SpanKind::Internal, {});
auto contextAndSpan = serviceTrace.CreateSpan("My API", {});
Azure::Core::Context callContext = std::move(contextAndSpan.first);
Request request(HttpMethod::Get, Url("https://www.microsoft.com"));

Expand Down Expand Up @@ -230,11 +228,10 @@ TEST(RequestActivityPolicy, TryRetries)

Azure::Core::_internal::ClientOptions clientOptions;
clientOptions.Telemetry.TracingProvider = testTracer;
Azure::Core::Tracing::_internal::DiagnosticTracingFactory serviceTrace(
Azure::Core::Tracing::_internal::ContextAndSpanFactory serviceTrace(
clientOptions, "my-service-cpp", "1.0b2");

auto contextAndSpan = serviceTrace.CreateSpan(
"My API", Azure::Core::Tracing::_internal::SpanKind::Internal, {});
auto contextAndSpan = serviceTrace.CreateSpan("My API", {});
Azure::Core::Context callContext = std::move(contextAndSpan.first);
Request request(HttpMethod::Get, Url("https://www.microsoft.com"));

Expand Down
Loading