Skip to content
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

Jump to bottom

OpenTelemetry API Review Feedback #3687

Merged
merged 4 commits into from
Jun 2, 2022
Merged

OpenTelemetry API Review Feedback #3687

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 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
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
50 changes: 49 additions & 1 deletion 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 @@ -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<Azure::Core::Tracing::OpenTelemetry::OpenTelemetryProvider>(
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)
{
{
Expand Down Expand Up @@ -635,7 +683,7 @@ TEST_F(OpenTelemetryServiceTests, ServiceApiImplementation)

VerifySpan(spans[0], R"(
{
"name": "HTTP GET #0",
"name": "HTTP GET",
"kind": "client",
"statusCode": "unset",
"attributes": {
Expand Down
14 changes: 6 additions & 8 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 @@ -180,12 +180,6 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
// using TracingContext = std::pair<std::shared_ptr<Span>, std::shared_ptr<Tracer>>;
using TracingContext = std::shared_ptr<Span>;

static DiagnosticTracingFactory* DiagnosticFactoryFromContext(
Azure::Core::Context const& context);

static Azure::Nullable<TracingContext> TracingContextFromContext(
Azure::Core::Context const& context);

public:
DiagnosticTracingFactory(
Azure::Core::_internal::ClientOptions const& options,
Expand All @@ -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.
Expand All @@ -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<Azure::Core::Tracing::_internal::AttributeSet> CreateAttributeSet();

static std::unique_ptr<DiagnosticTracingFactory> DiagnosticFactoryFromContext(
Azure::Core::Context const& context);
};

/**
Expand Down
98 changes: 54 additions & 44 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 @@ -23,68 +23,78 @@ std::unique_ptr<RawResponse> 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason or benefit of using stringstream here for building up the string, rather than just appending/concatenating regular std::string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally there were 4 elements being concatenated, one of which was an integer. stringstream was the best solution for that case.

Now it's not required but realistically the difference in complexity is probably negligible (adding two strings creates two temporary std::string objects and then assigns their concatenation to a 3rd, using stringstream allocates a buffer, then reallocates the buffer and then creates a std::string from that buffer - probably identical performance characteristics).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.
It stood out to me because I recall @BillyONeal or @CaseyCarter mention a while back to only use stringstream for formatting and not for string building scenarios like this. But I might be misremembering the detail (or reason).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that std::string does not have the C# usual "append becomes N^2" issue: const std::string ~= C# string, std::string ~= C# StringBuilder.

stringstream is less efficient in general than using std::string::append or std::string::push_back directly (where possible to do so). But you can't format numbers or things like that with it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this example, the two <<s are 0 or 1 allocation each (depending on length), then the str() is always an extra allocation. If this were instead auto ss = "HTTP " + request.GetMethod().ToString();, there is 0 or 1 allocation for the + and no second allocation on use.

Copy link
Collaborator

@BillyONeal BillyONeal Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Note that there's no temporary string constructed for the "HTTP ", you call this operator+, assuming ToString returns a prvalue:

https://github.com/microsoft/STL/blob/17fde2cbab6e8724d81c9555237c9a623d7fb954/stl/inc/xstring#L5053-L5057

)

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();
LarryOsterman marked this conversation as resolved.
Show resolved Hide resolved
// 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);
LarryOsterman marked this conversation as resolved.
Show resolved Hide resolved

std::string sanitizedUrl = m_inputSanitizer.SanitizeUrl(request.GetUrl()).GetAbsoluteUrl();
LarryOsterman marked this conversation as resolved.
Show resolved Hide resolved
createOptions.Attributes->AddAttribute("http.url", sanitizedUrl);
Azure::Nullable<std::string> 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<int>(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<int>(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);
}
}
70 changes: 32 additions & 38 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 @@ -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;
Expand All @@ -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<Span> newSpan(m_serviceTracer->CreateSpan(methodName, createOptions));
TracingContext tracingContext = newSpan;
Azure::Core::Context newContext = contextToUse.WithValue(ContextSpanKey, tracingContext);
Expand All @@ -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::TracingContext>
DiagnosticTracingFactory::TracingContextFromContext(Azure::Core::Context const& context)
{
TracingContext traceContext;
if (context.TryGetValue(ContextSpanKey, traceContext))
{
return traceContext;
}
else
{
return Azure::Nullable<TracingContext>{};
}
}

DiagnosticTracingFactory* DiagnosticTracingFactory::DiagnosticFactoryFromContext(
std::unique_ptr<DiagnosticTracingFactory> DiagnosticTracingFactory::DiagnosticFactoryFromContext(
Azure::Core::Context const& context)
{
DiagnosticTracingFactory* factory;
if (context.TryGetValue(TracingFactoryContextKey, factory))
{
return factory;
return std::make_unique<DiagnosticTracingFactory>(*factory);
}
else
{
Expand All @@ -116,10 +107,13 @@ namespace Azure { namespace Core { namespace Tracing { namespace _internal {
std::unique_ptr<Azure::Core::Tracing::_internal::AttributeSet>
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
Loading