Skip to content

Conversation

@jbogard
Copy link
Contributor

@jbogard jbogard commented Dec 2, 2020

Summary of the changes (Less than 80 chars)

  • Adds support of W3C Baggage standard

Addresses #28319

@mkArtakMSFT mkArtakMSFT added community-contribution Indicates that the PR has been added by a community member area-hosting labels Dec 2, 2020
public static readonly string AltSvc = "Alt-Svc";
public static readonly string Authority = ":authority";
public static readonly string Authorization = "Authorization";
public static readonly string Baggage = "baggage";
Copy link
Member

Choose a reason for hiding this comment

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

All other headers are pascal case. This won't matter when getting the value from the headers collection - it is case-insensitive.

Suggested change
public static readonly string Baggage = "baggage";
public static readonly string Baggage = "Baggage";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was curious about that. I kept it consistent with traceparent and tracestate, the other two headers related to this standard (W3C Trace Context) that are lowercase in this class.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Perhaps better as is then.

Copy link
Member

Choose a reason for hiding this comment

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

Since HTTP/2 all header names are officially lower case so it might make sense to start doing that with new entries. You're right though that the collections are all ignore-case so it shouldn't make a difference.

public static readonly string AltSvc = "Alt-Svc";
public static readonly string Authority = ":authority";
public static readonly string Authorization = "Authorization";
public static readonly string Baggage = "baggage";
Copy link
Member

Choose a reason for hiding this comment

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

Since HTTP/2 all header names are officially lower case so it might make sense to start doing that with new entries. You're right though that the collections are all ignore-case so it shouldn't make a difference.

@Tratcher Tratcher requested a review from shirhatti December 3, 2020 23:43
@Tratcher
Copy link
Member

Tratcher commented Dec 3, 2020

@shirhatti weren't we already planning on doing something like this?

@Tratcher Tratcher self-assigned this Dec 3, 2020
@Tratcher
Copy link
Member

Tratcher commented Dec 7, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@shirhatti
Copy link
Contributor

@shirhatti weren't we already planning on doing something like this?

Yes. We're planning to add support for the OpenTelemetry propagators API- https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/context/api-propagators.md

We'd still want to W3C trace context and W3C baggage propagation on by default with new API to configure/disable propagation. This change would be needed even if we add propagators support.

@Tratcher Tratcher merged commit 2f64d67 into dotnet:master Dec 15, 2020
@Tratcher
Copy link
Member

Thanks

@ghost
Copy link

ghost commented Dec 15, 2020

Hi @Tratcher. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@Tratcher Tratcher added this to the 6.0-preview1 milestone Dec 15, 2020
@jbogard jbogard deleted the support-w3c-baggage branch December 15, 2020 18:57
@amcasey amcasey added the area-hosting Includes Hosting label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-hosting Includes Hosting community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants