Azure / Azure/azure-sdk-for-cpp

TracingContextFactory allocates when no tracer is configured

Open
#7,340 2 comments 0 reactions 1 assignee Claimed by @xirzec View on GitHub
Azure.Core Client needs-team-attention
Dominant language
C++
Stars
205
Forks
172
Avg merge
1d 3h
Merged PRs (30d)
37

Description

## Summary

`Azure::Core::Tracing::_internal::TracingContextFactory::CreateTracingContext` allocates on the no-tracer path. Every call that receives a caller context without the factory performs two heap allocations, even when the application configured no tracing provider. A service that does not use the HTTP pipeline pays that cost for every top-level operation and gets nothing back.

## Motivation

`CreateTracingContext` calls `context.WithValue(TracingFactoryContextKey, this)` at `sdk/core/azure-core/src/tracing/tracing.cpp:66-68` before it tests `HasTracer()`, and `Context::WithValue` performs two `make_shared` calls at `sdk/core/azure-core/inc/azure/core/context.hpp:289-293`. The comment at `tracing.cpp:61-64` justifies the unconditional injection by saying the factory also builds the User-Agent header. That justification is stale. At commit 99f45a5f1 `RequestActivityPolicy` read `GetUserAgent()` from the injected factory, but `GetUserAgent` no longer exists anywhere in `sdk/core`, and `TelemetryPolicy` now produces the header on its own at `sdk/core/azure-core/src/http/telemetry_policy.cpp:16-21`. The only remaining consumer of the injected factory is `RequestActivityPolicy`, which acts only when `tracingFactory && tracingFactory->HasTracer()` at `sdk/core/azure-core/src/http/request_activity_policy.cpp:28`, so a factory without a tracer has no observable effect. `WithValue` returns a child context and never changes the caller context, so two calls that share one caller context both miss the guard and both allocate. An AMQP service such as `azure-messaging-eventhubs` builds a fresh caller context for each operation, which measures at two allocations for every send and every receive.

## Proposal

- Return `TracingContext{context, ServiceSpan{}}` at the top of `CreateTracingContext` in `sdk/core/azure-core/src/tracing/tracing.cpp` when `HasTracer()` is false, and delete the dead `else` branch at lines 98-101. Copying the context copies one `shared_ptr` and does not reach the heap.
- Keep the guarded injection and the span creation unchanged for the tracer-configured path, so HTTP clients behave exactly as they do today.
- Replace the stale comment at `tracing.cpp:61-64` with one that names `RequestActivityPolicy` as the consumer and records that `TelemetryPolicy` owns the User-Agent header.
- Correct the `TelemetryPolicy` doc comment at `sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp:546-550`, which claims User-Agent generation moved into `RequestActivityPolicy`. That merger was reverted, and the stale claim invites a future reader to restore the injection.
- Add a test near `BasicServiceSpanTests` in `sdk/core/azure-core/test/ut/service_tracing_test.cpp` that makes sure `CreateFromContext` returns null for a context built with no provider, and returns a factory with `HasTracer()` true when a provider is set.
- Add a CHANGELOG entry under "Other Changes" recording that `CreateTracingContext` no longer allocates when no tracer is configured.

Rejected alternatives. An opt-out constructor flag works, but it adds a data member to a class whose inline definition ships in a public header, and it forces every non-HTTP service to opt in by hand. Passing the factory to `RequestActivityPolicy` outside the context would let the injection be deleted for everyone, but the pipeline constructor carries no service name, which `az.namespace` needs, so it requires plumbing through every service that builds a pipeline. That route remains the right long-term direction if User-Agent duty ever returns to `RequestActivityPolicy`.

## Validation

- Both symbols live in `Azure::Core::Tracing::_internal`, and a repository-wide search for `TracingFactoryContextKey` and `CreateFromContext` finds one non-test consumer, so no public header changes and no API or ABI break.
- Every existing `RequestActivityPolicy` test configures a tracing provider, at `sdk/core/azure-core/test/ut/request_activity_policy_test.cpp:161-246`, so no current test pins the no-tracer injection.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.