Skip to content

Allow Trace Samplers to Modify the Activity Trace State - #65530

Merged
tarekgh merged 2 commits into
dotnet:mainfrom
tarekgh:SupportTraceStateDuringSampling
Feb 19, 2022
Merged

Allow Trace Samplers to Modify the Activity Trace State#65530
tarekgh merged 2 commits into
dotnet:mainfrom
tarekgh:SupportTraceStateDuringSampling

Conversation

@tarekgh

@tarekghtarekgh commented Feb 17, 2022

Copy link
Copy Markdown
Member

Fixes#63652

@ghostghost assigned tarekghFeb 17, 2022
@ghost

ghost commented Feb 17, 2022

Copy link
Copy Markdown

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 Feb 17, 2022

Copy link
Copy Markdown

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

Issue Details

Fixes #63652

Author:tarekgh
Assignees:tarekgh
Labels:

area-System.Diagnostics.Tracing, new-api-needs-documentation

Milestone:-

@ghost

ghost commented Feb 17, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #63652

Author:tarekgh
Assignees:tarekgh
Labels:

new-api-needs-documentation, area-System.Diagnostics.Activity

Milestone:-

@tarekghtarekgh added this to the 7.0.0 milestone Feb 17, 2022
@tarekgh

tarekgh commented Feb 17, 2022

Copy link
Copy Markdown
MemberAuthor

@noahfalk could you please help reviewing this?

CC @ericstj for the System.Runtime.CompilerServices.IsExternalInit type forward.

CC @cijothomas@reyang

reyang
reyang approved these changes Feb 18, 2022
View reviewed changes

@reyangreyang left a comment

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.

LGTM.

noahfalk
noahfalk approved these changes Feb 18, 2022
View reviewed changes

@noahfalknoahfalk 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 : )

Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs
{
if (setTraceWithStringParent)
{
activityOptions = activityOptions with { TraceState = traceStateValueWithStringParent };

@noahfalknoahfalkFeb 18, 2022

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.

More of a design question... if I was a developer using an earlier version of the C# compiler, would I have any way to achieve the same result?

@tarekghtarekghFeb 18, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No, to use this feature must use C# compiler version 10 or later. This is not the first feature we expose in the framework using this new syntax. System.Text.Json already using it a lot.

@noahfalknoahfalkFeb 19, 2022

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.

I assume @reyang and @cijothomas are OK with this requirement? Effectively they will have to compile OpenTelemetry SDK with a compiler version >= 10.

@cijothomascijothomasFeb 23, 2022

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.

yes. OpenTelemetry SDK uses 10

Comment threadsrc/libraries/System.Diagnostics.DiagnosticSource/tests/ActivitySourceTests.cs
@tarekgh
tarekgh merged commit 5d296d0 into dotnet:mainFeb 19, 2022
@tarekgh
tarekgh deleted the SupportTraceStateDuringSampling branch February 19, 2022 00:46
ericstj
ericstj reviewed Feb 23, 2022
View reviewed changes

@ericstjericstj 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

@tarekghtarekgh mentioned this pull request Mar 4, 2022
Closed
@ghostghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@noahfalknoahfalknoahfalk approved these changes
@ericstjericstjericstj left review comments
+2 more reviewers
@cijothomascijothomascijothomas left review comments
@reyangreyangreyang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tarekghtarekgh

Labels

area-System.Diagnostics.Activitynew-api-needs-documentation

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Samplers should be allowed to modify tracestate

5 participants

@tarekgh@cijothomas@noahfalk@ericstj@reyang