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.

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

[Core] Fix traceparent header in DistributedTracingPolicy #40074

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pvaneck
Copy link
Member

@pvaneck pvaneck commented Mar 14, 2025

When we get the trace context headers, we should first ensure that we are inside the HTTP span's context to ensure that the traceparent header corresponds to the HTTP span's context.

@pvaneck pvaneck force-pushed the core-traceparent-context branch from f1b560e to 1705558 Compare March 14, 2025 02:24
@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

pvaneck added 2 commits March 14, 2025 19:45
When get the trace context headers, we should first ensure that we are
inside the HTTP span's context to ensure that the traceparent header
contains the correct span ID.

Signed-off-by: Paul Van Eck <[email protected]>
Signed-off-by: Paul Van Eck <[email protected]>
@pvaneck pvaneck force-pushed the core-traceparent-context branch from 1705558 to 2917ae9 Compare March 14, 2025 19:45
@pvaneck pvaneck marked this pull request as ready for review March 15, 2025 02:05
@Copilot Copilot bot review requested due to automatic review settings March 15, 2025 02:05

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the traceparent header generation in the DistributedTracingPolicy so that the header reflects the HTTP span context correctly. Key changes include updating tests to verify the new header format, modifying the tracing_common header logic, and adjusting the distributed tracing policy to use the proper context manager.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
sdk/core/azure-core/tests/test_tracing_policy.py Updated test assertions to check the new traceparent header format and added assertions to validate span IDs with OpenTelemetry formatting.
sdk/core/azure-core/tests/tracing_common.py Changed the span_instance method implementation by removing the _span attribute and using self instead, and adjusted context management accordingly.
sdk/core/azure-core/azure/core/pipeline/policies/_distributed_tracing.py Updated request handling with the new change_context pattern and modified the assignment of the tracing context.
sdk/core/azure-core/CHANGELOG.md Updated changelog to document the fix in the traceparent header.
sdk/core/azure-core/azure/core/tracing/opentelemetry.py Refined type hints for suppression-related methods.
Comments suppressed due to low confidence (3)

sdk/core/azure-core/tests/tracing_common.py:37

  • Removing the assignment to self._span and switching to returning self in span_instance may affect code that relies on the previous behavior. Please verify that this change is intentional and update any related documentation or usage accordingly.
self._span = span

sdk/core/azure-core/azure/core/pipeline/policies/_distributed_tracing.py:160

  • There appears to be a duplicate assignment of otel_span to request.context[self.TRACING_CONTEXT]. Consider removing the redundant assignment for clarity and to avoid potential side-effects.
request.context[self.TRACING_CONTEXT] = otel_span

sdk/core/azure-core/tests/test_tracing_policy.py:386

  • The variable 'traceparent' is used without being defined in this test context. Please ensure that 'traceparent' is retrieved from the correct source, such as the request headers, before splitting its value.
assert traceparent.split("-")[1] == format_trace_id(span_context.trace_id)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants