Skip to content

Add ActivityContext parsing APIs - #40183

Merged
tarekgh merged 2 commits into
dotnet:masterfrom
tarekgh:ActivityContextParsingAPIs
Aug 3, 2020
Merged

Add ActivityContext parsing APIs#40183
tarekgh merged 2 commits into
dotnet:masterfrom
tarekgh:ActivityContextParsingAPIs

Conversation

@tarekgh

@tarekgh tarekgh commented Jul 31, 2020

Copy link
Copy Markdown
Member

Fixes #40046

@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost

ghost commented Jul 31, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

@ghost

ghost commented Jul 31, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

@tarekgh tarekgh added this to the 5.0.0 milestone Jul 31, 2020
@tarekgh

tarekgh commented Jul 31, 2020

Copy link
Copy Markdown
Member Author

/// Parse W3C trace parent and state headers to ActivityContext object.
/// </summary>
/// <param name="traceParent">W3C trace parent header.</param>
/// <param name="traceState">Trace state.</param>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nits: Kind of odd to have W3C in the one comment but not the other. How about...

        /// <param name="traceParent">W3C traceparent header value.</param>
        /// <param name="traceState">W3C tracestate header value.</param>

Personally not a fan of the casing "traceparent" & "tracestate" but that's how they are named in the spec.

For the method summary, might want to refer to them collectively as Trace Context? Eg:

        /// <summary>
        /// Parse W3C Trace Context header values into an ActivityContext object.
        /// </summary>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Personally not a fan of the casing "traceparent" & "tracestate" but that's how they are named in the spec.

I prefer to case it as the rest of C# method parameters.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks @CodeBlanch I have fixed the comments as you suggested.

@noahfalk noahfalk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

@tarekgh

tarekgh commented Aug 3, 2020

Copy link
Copy Markdown
Member Author

The failed tests are not related and tracked. merging this one.

Thanks all for your reviews.

@tarekgh
tarekgh merged commit a4f15bf into dotnet:master Aug 3, 2020
@tarekgh
tarekgh deleted the ActivityContextParsingAPIs branch August 3, 2020 21:03
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
* Add ActivityContext parsing APIs

* Address the feedback
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Diagnostics.ActivitySource API Addition

5 participants