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

Set the baggage even when the trace id is not successfully extracted #55341

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

Conversation

joegoldman2
Copy link
Contributor

@joegoldman2 joegoldman2 commented Apr 24, 2024

Fixes #50158.

Better to review it without whitespace diff: https://github.com/dotnet/aspnetcore/pull/55341/files?diff=unified&w=1.

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Apr 24, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 24, 2024
Copy link
Contributor

Thanks for your PR, @joegoldman2. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

Copy link
Member

@amcasey amcasey left a comment

Choose a reason for hiding this comment

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

Some basic questions.

@@ -535,10 +535,39 @@ public void ActivityBaggageReadFromLegacyHeaders()
features.Set<IHttpRequestFeature>(new HttpRequestFeature()
{
Headers = new HeaderDictionary()
{
{"baggage", "Key1=value1, Key2=value2"}
Copy link
Member

Choose a reason for hiding this comment

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

Not having a request id here exercises the new code (thanks!), but is it something that could plausibly happen? This question may be terribly naive - I'm not very familiar with distributed tracing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's possible. Not all frameworks set it. This is not the case with .NET because HttpClient sets it (https://github.com/dotnet/runtime/blob/33a68d0cef067034df5cbac89415568e5fda63ce/src/libraries/System.Net.Http/src/System/Net/Http/DiagnosticsHandler.cs#L308) but someone can use another framework (or make the request manually via curl, Postman, or whatever) and in this case there will be no request id (traceparent) in the request (ASP.NET will generate one in this case).

Copy link
Contributor

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label May 6, 2024
@joegoldman2 joegoldman2 requested a review from amcasey May 7, 2024 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Baggage not extracted without trace id from the request
3 participants