Skip to content

Support W3C Propagator #114583

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

Merged
merged 13 commits into from
Apr 16, 2025
Merged

Support W3C Propagator #114583

merged 13 commits into from
Apr 16, 2025

Conversation

tarekgh
Copy link
Member

@tarekgh tarekgh commented Apr 12, 2025

Contribute to #103174

@Copilot Copilot AI review requested due to automatic review settings April 12, 2025 00:03
@tarekgh tarekgh added this to the 10.0.0 milestone Apr 12, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 6 changed files in this pull request and generated 1 comment.

Files not reviewed (2)
  • src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj: Language not supported
  • src/libraries/System.Diagnostics.DiagnosticSource/tests/System.Diagnostics.DiagnosticSource.Tests.csproj: Language not supported

@tarekgh tarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Apr 12, 2025
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 12, 2025
Copy link
Contributor

dotnet-policy-service bot commented Apr 12, 2025

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@tarekgh
Copy link
Member Author

tarekgh commented Apr 12, 2025

@tarekgh tarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Apr 12, 2025
@tarekgh
Copy link
Member Author

tarekgh commented Apr 13, 2025

@MihaZupan @antonfirsov we are changing the default propagator to use the new W3C one instead of the legacy one. Looks like many http/socket tests are failing because of that. Could you please help by looking at the failed tests and fix these?

The new W3C propagator is not supporting Request-Id anymore and only supports traceparent header. Legacy activities created with hierarchical id formats will not be propagated using the new W3C propagator. Additionally, the new W3C is more restricted to the W3C specification so most likely tests can be using some test values which are not compliant with the specs.

If want to continue testing hierarchical activity ids, you can consider reset the current propagator to the legacy one. We haven't exposed API to return that yet, but the test can use reflection if want to. Let me know and I can help.

@tarekgh
Copy link
Member Author

tarekgh commented Apr 14, 2025

CC @JamesNK for awareness.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

A couple suggestions inline but other than that LGTM!

@JamesNK
Copy link
Member

JamesNK commented Apr 14, 2025

Does ASP.NET Core need to do anything to read headers added to requests? Or does it already work?

@tarekgh
Copy link
Member Author

tarekgh commented Apr 14, 2025

Does ASP.NET Core need to do anything to read headers added to requests? Or does it already work?

aspnet core support both header names baggage and Correlation-Context, so I expect will still continue without a problem. We need to watch if we run into any issue though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics.Activity breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants