[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) - #111289

Merged
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix
Jan 14, 2025
Merged

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s)#111289
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix

Conversation

@CodeBlanch

Copy link
Copy Markdown
Contributor

Changes

  • Reset ActivityTraceFlags.Recorded when creating activity if not requested by sampler(s) because it may be inherited from a parent

Details

[Ran into this while working on #6058.]

Scenario:

  • We have an incoming request with traceparent marked as Recorded.
  • OTel sampler decides to drop for whatever reason.
  • Because this is a root, even though a drop was requested, OTel SDK will still create a propagation-only span/activity.
  • Here is the issue: The created span/activity inherits recorded from the parent context and therefore has Recorded=true. A propagation-only span should have Recorded=false.

/cc @tarekgh@noahfalk@samsp-msft@cijothomas@alanwest

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 10, 2025
@tarekghtarekgh added this to the 10.0.0 milestone Jan 10, 2025
@tarekgh

Copy link
Copy Markdown
Member
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?


Refers to: src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs:1236 in 5d2e54d. [](commit_id = 5d2e54d, deletion_comment = False)

@tarekgh
tarekgh requested a review from noahfalkJanuary 10, 2025 22:42
@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?

@tarekgh I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

Kind of hard to say.

Here are some thoughts...

The default sampler in OTel is ParentBased(AlwaysOn) which means first check the parent. If the parent is marked as Recorded (aka sampled) then respect that and create a recorded child. If there is no parent, create a recorded/sampled span (AlwaysOn).

So no one using the defaults will be impacted.

To be impacted users would have to switch the sampler or make a custom one which attempts to drop remote things. The bug is such that the drop isn't really respected. Recorded causes the spans to be exported (but they have IsAllDataRequested=false so they won't be populated). Impact is probably higher costs, maybe some screwy spans without data. What may be more interesting is that span, if it is propagated out of proc, will be transmitted as Recorded. So some other system downstream using ParentBased might decide to sample it fully essentially reversing the drop decision.

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

@tarekgh

Copy link
Copy Markdown
Member

I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

We don't have to remove the line if we are worried about the future adding to the flags. But we can just modify it to the following:

activity.ActivityTraceFlags=parentContext.TraceFlags&~ActivityTraceFlags.Recorded;

@tarekgh

Copy link
Copy Markdown
Member

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

@tarekghtarekgh 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.

@noahfalk

Copy link
Copy Markdown
Member

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

Yeah, I think a breaking change notice would be warranted here. For downlevel what is the plan? Just warn users that they won't be able to disable the recorded flag?

@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch could you please file the breaking change in https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml? Thanks!

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@tarekgh Done, see link above ^

@tarekghtarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jan 14, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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.

@dotnet-policy-servicedotnet-policy-serviceBot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 14, 2025
@tarekgh

Copy link
Copy Markdown
Member

/ba-g the failure is known build timeout issue happening in other PRs too.

@tarekgh
tarekgh merged commit 7132268 into dotnet:mainJan 14, 2025
Comment on lines -1236 to +1238
activity.ActivityTraceFlags = parentContext.TraceFlags;
// Note: Don't inherit Recorded from parent as it is set below
// based on sampling decision
activity.ActivityTraceFlags = parentContext.TraceFlags & ~ActivityTraceFlags.Recorded;

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.

Why does this line exist at all? If W3C trace context introduces new flags, what is to say that inheriting the flags from the parent will be the correct thing to do for those new flags?

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.

Ah, I see this was already discussed... removing the line seems more correct to me...

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.

In looking at the latest editor's draft of the spec, there is a new flag proposed: Random Trace ID Flag. My read of this flag is that it actually should be inherited from the parent, but there's no way to be certain this will be true for all new flags.

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.

Keeping this line is not hurting for now. Any newly introduced flag will need to be decided if it is inheritable from the parent or not anyway.

@ericstj

Copy link
Copy Markdown
Member

📋 Breaking Change Documentation Required

Create a breaking change issue with AI-generated content

Generated by Breaking Change Documentation Tool - 2025-10-03 13:49:29

@tarekgh

Copy link
Copy Markdown
Member

@ericstj the breaking change for this is already submitted and finalized dotnet/docs#44282.

@tarekghtarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 3, 2025
@ericstj

Copy link
Copy Markdown
Member

Thank you @tarekgh

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Activitybreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.community-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@CodeBlanch@tarekgh@noahfalk@ericstj@alanwest@cijothomas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) - #111289

Merged
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix
Jan 14, 2025
Merged

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s)#111289
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix

Conversation

@CodeBlanch

Copy link
Copy Markdown
Contributor

Changes

  • Reset ActivityTraceFlags.Recorded when creating activity if not requested by sampler(s) because it may be inherited from a parent

Details

[Ran into this while working on #6058.]

Scenario:

  • We have an incoming request with traceparent marked as Recorded.
  • OTel sampler decides to drop for whatever reason.
  • Because this is a root, even though a drop was requested, OTel SDK will still create a propagation-only span/activity.
  • Here is the issue: The created span/activity inherits recorded from the parent context and therefore has Recorded=true. A propagation-only span should have Recorded=false.

/cc @tarekgh@noahfalk@samsp-msft@cijothomas@alanwest

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 10, 2025
@tarekghtarekgh added this to the 10.0.0 milestone Jan 10, 2025
@tarekgh

Copy link
Copy Markdown
Member
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?


Refers to: src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs:1236 in 5d2e54d. [](commit_id = 5d2e54d, deletion_comment = False)

@tarekgh
tarekgh requested a review from noahfalkJanuary 10, 2025 22:42
@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?

@tarekgh I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

Kind of hard to say.

Here are some thoughts...

The default sampler in OTel is ParentBased(AlwaysOn) which means first check the parent. If the parent is marked as Recorded (aka sampled) then respect that and create a recorded child. If there is no parent, create a recorded/sampled span (AlwaysOn).

So no one using the defaults will be impacted.

To be impacted users would have to switch the sampler or make a custom one which attempts to drop remote things. The bug is such that the drop isn't really respected. Recorded causes the spans to be exported (but they have IsAllDataRequested=false so they won't be populated). Impact is probably higher costs, maybe some screwy spans without data. What may be more interesting is that span, if it is propagated out of proc, will be transmitted as Recorded. So some other system downstream using ParentBased might decide to sample it fully essentially reversing the drop decision.

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

@tarekgh

Copy link
Copy Markdown
Member

I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

We don't have to remove the line if we are worried about the future adding to the flags. But we can just modify it to the following:

activity.ActivityTraceFlags=parentContext.TraceFlags&~ActivityTraceFlags.Recorded;

@tarekgh

Copy link
Copy Markdown
Member

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

@tarekghtarekgh 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.

@noahfalk

Copy link
Copy Markdown
Member

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

Yeah, I think a breaking change notice would be warranted here. For downlevel what is the plan? Just warn users that they won't be able to disable the recorded flag?

@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch could you please file the breaking change in https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml? Thanks!

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@tarekgh Done, see link above ^

@tarekghtarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jan 14, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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.

@dotnet-policy-servicedotnet-policy-serviceBot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 14, 2025
@tarekgh

Copy link
Copy Markdown
Member

/ba-g the failure is known build timeout issue happening in other PRs too.

@tarekgh
tarekgh merged commit 7132268 into dotnet:mainJan 14, 2025
Comment on lines -1236 to +1238
activity.ActivityTraceFlags = parentContext.TraceFlags;
// Note: Don't inherit Recorded from parent as it is set below
// based on sampling decision
activity.ActivityTraceFlags = parentContext.TraceFlags & ~ActivityTraceFlags.Recorded;

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.

Why does this line exist at all? If W3C trace context introduces new flags, what is to say that inheriting the flags from the parent will be the correct thing to do for those new flags?

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.

Ah, I see this was already discussed... removing the line seems more correct to me...

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.

In looking at the latest editor's draft of the spec, there is a new flag proposed: Random Trace ID Flag. My read of this flag is that it actually should be inherited from the parent, but there's no way to be certain this will be true for all new flags.

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.

Keeping this line is not hurting for now. Any newly introduced flag will need to be decided if it is inheritable from the parent or not anyway.

@ericstj

Copy link
Copy Markdown
Member

📋 Breaking Change Documentation Required

Create a breaking change issue with AI-generated content

Generated by Breaking Change Documentation Tool - 2025-10-03 13:49:29

@tarekgh

Copy link
Copy Markdown
Member

@ericstj the breaking change for this is already submitted and finalized dotnet/docs#44282.

@tarekghtarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 3, 2025
@ericstj

Copy link
Copy Markdown
Member

Thank you @tarekgh

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Activitybreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.community-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@CodeBlanch@tarekgh@noahfalk@ericstj@alanwest@cijothomas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) - #111289

Merged
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix
Jan 14, 2025
Merged

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s)#111289
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix

Conversation

@CodeBlanch

Copy link
Copy Markdown
Contributor

Changes

  • Reset ActivityTraceFlags.Recorded when creating activity if not requested by sampler(s) because it may be inherited from a parent

Details

[Ran into this while working on #6058.]

Scenario:

  • We have an incoming request with traceparent marked as Recorded.
  • OTel sampler decides to drop for whatever reason.
  • Because this is a root, even though a drop was requested, OTel SDK will still create a propagation-only span/activity.
  • Here is the issue: The created span/activity inherits recorded from the parent context and therefore has Recorded=true. A propagation-only span should have Recorded=false.

/cc @tarekgh@noahfalk@samsp-msft@cijothomas@alanwest

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 10, 2025
@tarekghtarekgh added this to the 10.0.0 milestone Jan 10, 2025
@tarekgh

Copy link
Copy Markdown
Member
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?


Refers to: src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs:1236 in 5d2e54d. [](commit_id = 5d2e54d, deletion_comment = False)

@tarekgh
tarekgh requested a review from noahfalkJanuary 10, 2025 22:42
@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?

@tarekgh I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

Kind of hard to say.

Here are some thoughts...

The default sampler in OTel is ParentBased(AlwaysOn) which means first check the parent. If the parent is marked as Recorded (aka sampled) then respect that and create a recorded child. If there is no parent, create a recorded/sampled span (AlwaysOn).

So no one using the defaults will be impacted.

To be impacted users would have to switch the sampler or make a custom one which attempts to drop remote things. The bug is such that the drop isn't really respected. Recorded causes the spans to be exported (but they have IsAllDataRequested=false so they won't be populated). Impact is probably higher costs, maybe some screwy spans without data. What may be more interesting is that span, if it is propagated out of proc, will be transmitted as Recorded. So some other system downstream using ParentBased might decide to sample it fully essentially reversing the drop decision.

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

@tarekgh

Copy link
Copy Markdown
Member

I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

We don't have to remove the line if we are worried about the future adding to the flags. But we can just modify it to the following:

activity.ActivityTraceFlags=parentContext.TraceFlags&~ActivityTraceFlags.Recorded;

@tarekgh

Copy link
Copy Markdown
Member

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

@tarekghtarekgh 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.

@noahfalk

Copy link
Copy Markdown
Member

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

Yeah, I think a breaking change notice would be warranted here. For downlevel what is the plan? Just warn users that they won't be able to disable the recorded flag?

@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch could you please file the breaking change in https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml? Thanks!

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@tarekgh Done, see link above ^

@tarekghtarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jan 14, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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.

@dotnet-policy-servicedotnet-policy-serviceBot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 14, 2025
@tarekgh

Copy link
Copy Markdown
Member

/ba-g the failure is known build timeout issue happening in other PRs too.

@tarekgh
tarekgh merged commit 7132268 into dotnet:mainJan 14, 2025
Comment on lines -1236 to +1238
activity.ActivityTraceFlags = parentContext.TraceFlags;
// Note: Don't inherit Recorded from parent as it is set below
// based on sampling decision
activity.ActivityTraceFlags = parentContext.TraceFlags & ~ActivityTraceFlags.Recorded;

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.

Why does this line exist at all? If W3C trace context introduces new flags, what is to say that inheriting the flags from the parent will be the correct thing to do for those new flags?

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.

Ah, I see this was already discussed... removing the line seems more correct to me...

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.

In looking at the latest editor's draft of the spec, there is a new flag proposed: Random Trace ID Flag. My read of this flag is that it actually should be inherited from the parent, but there's no way to be certain this will be true for all new flags.

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.

Keeping this line is not hurting for now. Any newly introduced flag will need to be decided if it is inheritable from the parent or not anyway.

@ericstj

Copy link
Copy Markdown
Member

📋 Breaking Change Documentation Required

Create a breaking change issue with AI-generated content

Generated by Breaking Change Documentation Tool - 2025-10-03 13:49:29

@tarekgh

Copy link
Copy Markdown
Member

@ericstj the breaking change for this is already submitted and finalized dotnet/docs#44282.

@tarekghtarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 3, 2025
@ericstj

Copy link
Copy Markdown
Member

Thank you @tarekgh

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Activitybreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.community-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@CodeBlanch@tarekgh@noahfalk@ericstj@alanwest@cijothomas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) - #111289

Merged
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix
Jan 14, 2025
Merged

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s)#111289
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix

Conversation

@CodeBlanch

Copy link
Copy Markdown
Contributor

Changes

  • Reset ActivityTraceFlags.Recorded when creating activity if not requested by sampler(s) because it may be inherited from a parent

Details

[Ran into this while working on #6058.]

Scenario:

  • We have an incoming request with traceparent marked as Recorded.
  • OTel sampler decides to drop for whatever reason.
  • Because this is a root, even though a drop was requested, OTel SDK will still create a propagation-only span/activity.
  • Here is the issue: The created span/activity inherits recorded from the parent context and therefore has Recorded=true. A propagation-only span should have Recorded=false.

/cc @tarekgh@noahfalk@samsp-msft@cijothomas@alanwest

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 10, 2025
@tarekghtarekgh added this to the 10.0.0 milestone Jan 10, 2025
@tarekgh

Copy link
Copy Markdown
Member
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?


Refers to: src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs:1236 in 5d2e54d. [](commit_id = 5d2e54d, deletion_comment = False)

@tarekgh
tarekgh requested a review from noahfalkJanuary 10, 2025 22:42
@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?

@tarekgh I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

Kind of hard to say.

Here are some thoughts...

The default sampler in OTel is ParentBased(AlwaysOn) which means first check the parent. If the parent is marked as Recorded (aka sampled) then respect that and create a recorded child. If there is no parent, create a recorded/sampled span (AlwaysOn).

So no one using the defaults will be impacted.

To be impacted users would have to switch the sampler or make a custom one which attempts to drop remote things. The bug is such that the drop isn't really respected. Recorded causes the spans to be exported (but they have IsAllDataRequested=false so they won't be populated). Impact is probably higher costs, maybe some screwy spans without data. What may be more interesting is that span, if it is propagated out of proc, will be transmitted as Recorded. So some other system downstream using ParentBased might decide to sample it fully essentially reversing the drop decision.

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

@tarekgh

Copy link
Copy Markdown
Member

I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

We don't have to remove the line if we are worried about the future adding to the flags. But we can just modify it to the following:

activity.ActivityTraceFlags=parentContext.TraceFlags&~ActivityTraceFlags.Recorded;

@tarekgh

Copy link
Copy Markdown
Member

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

@tarekghtarekgh 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.

@noahfalk

Copy link
Copy Markdown
Member

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

Yeah, I think a breaking change notice would be warranted here. For downlevel what is the plan? Just warn users that they won't be able to disable the recorded flag?

@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch could you please file the breaking change in https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml? Thanks!

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@tarekgh Done, see link above ^

@tarekghtarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jan 14, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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.

@dotnet-policy-servicedotnet-policy-serviceBot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 14, 2025
@tarekgh

Copy link
Copy Markdown
Member

/ba-g the failure is known build timeout issue happening in other PRs too.

@tarekgh
tarekgh merged commit 7132268 into dotnet:mainJan 14, 2025
Comment on lines -1236 to +1238
activity.ActivityTraceFlags = parentContext.TraceFlags;
// Note: Don't inherit Recorded from parent as it is set below
// based on sampling decision
activity.ActivityTraceFlags = parentContext.TraceFlags & ~ActivityTraceFlags.Recorded;

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.

Why does this line exist at all? If W3C trace context introduces new flags, what is to say that inheriting the flags from the parent will be the correct thing to do for those new flags?

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.

Ah, I see this was already discussed... removing the line seems more correct to me...

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.

In looking at the latest editor's draft of the spec, there is a new flag proposed: Random Trace ID Flag. My read of this flag is that it actually should be inherited from the parent, but there's no way to be certain this will be true for all new flags.

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.

Keeping this line is not hurting for now. Any newly introduced flag will need to be decided if it is inheritable from the parent or not anyway.

@ericstj

Copy link
Copy Markdown
Member

📋 Breaking Change Documentation Required

Create a breaking change issue with AI-generated content

Generated by Breaking Change Documentation Tool - 2025-10-03 13:49:29

@tarekgh

Copy link
Copy Markdown
Member

@ericstj the breaking change for this is already submitted and finalized dotnet/docs#44282.

@tarekghtarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 3, 2025
@ericstj

Copy link
Copy Markdown
Member

Thank you @tarekgh

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Activitybreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.community-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@CodeBlanch@tarekgh@noahfalk@ericstj@alanwest@cijothomas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) - #111289

Merged
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix
Jan 14, 2025
Merged

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s)#111289
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix

Conversation

@CodeBlanch

Copy link
Copy Markdown
Contributor

Changes

  • Reset ActivityTraceFlags.Recorded when creating activity if not requested by sampler(s) because it may be inherited from a parent

Details

[Ran into this while working on #6058.]

Scenario:

  • We have an incoming request with traceparent marked as Recorded.
  • OTel sampler decides to drop for whatever reason.
  • Because this is a root, even though a drop was requested, OTel SDK will still create a propagation-only span/activity.
  • Here is the issue: The created span/activity inherits recorded from the parent context and therefore has Recorded=true. A propagation-only span should have Recorded=false.

/cc @tarekgh@noahfalk@samsp-msft@cijothomas@alanwest

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 10, 2025
@tarekghtarekgh added this to the 10.0.0 milestone Jan 10, 2025
@tarekgh

Copy link
Copy Markdown
Member
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?


Refers to: src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs:1236 in 5d2e54d. [](commit_id = 5d2e54d, deletion_comment = False)

@tarekgh
tarekgh requested a review from noahfalkJanuary 10, 2025 22:42
@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?

@tarekgh I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

Kind of hard to say.

Here are some thoughts...

The default sampler in OTel is ParentBased(AlwaysOn) which means first check the parent. If the parent is marked as Recorded (aka sampled) then respect that and create a recorded child. If there is no parent, create a recorded/sampled span (AlwaysOn).

So no one using the defaults will be impacted.

To be impacted users would have to switch the sampler or make a custom one which attempts to drop remote things. The bug is such that the drop isn't really respected. Recorded causes the spans to be exported (but they have IsAllDataRequested=false so they won't be populated). Impact is probably higher costs, maybe some screwy spans without data. What may be more interesting is that span, if it is propagated out of proc, will be transmitted as Recorded. So some other system downstream using ParentBased might decide to sample it fully essentially reversing the drop decision.

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

@tarekgh

Copy link
Copy Markdown
Member

I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

We don't have to remove the line if we are worried about the future adding to the flags. But we can just modify it to the following:

activity.ActivityTraceFlags=parentContext.TraceFlags&~ActivityTraceFlags.Recorded;

@tarekgh

Copy link
Copy Markdown
Member

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

@tarekghtarekgh 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.

@noahfalk

Copy link
Copy Markdown
Member

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

Yeah, I think a breaking change notice would be warranted here. For downlevel what is the plan? Just warn users that they won't be able to disable the recorded flag?

@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch could you please file the breaking change in https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml? Thanks!

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@tarekgh Done, see link above ^

@tarekghtarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jan 14, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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.

@dotnet-policy-servicedotnet-policy-serviceBot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 14, 2025
@tarekgh

Copy link
Copy Markdown
Member

/ba-g the failure is known build timeout issue happening in other PRs too.

@tarekgh
tarekgh merged commit 7132268 into dotnet:mainJan 14, 2025
Comment on lines -1236 to +1238
activity.ActivityTraceFlags = parentContext.TraceFlags;
// Note: Don't inherit Recorded from parent as it is set below
// based on sampling decision
activity.ActivityTraceFlags = parentContext.TraceFlags & ~ActivityTraceFlags.Recorded;

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.

Why does this line exist at all? If W3C trace context introduces new flags, what is to say that inheriting the flags from the parent will be the correct thing to do for those new flags?

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.

Ah, I see this was already discussed... removing the line seems more correct to me...

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.

In looking at the latest editor's draft of the spec, there is a new flag proposed: Random Trace ID Flag. My read of this flag is that it actually should be inherited from the parent, but there's no way to be certain this will be true for all new flags.

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.

Keeping this line is not hurting for now. Any newly introduced flag will need to be decided if it is inheritable from the parent or not anyway.

@ericstj

Copy link
Copy Markdown
Member

📋 Breaking Change Documentation Required

Create a breaking change issue with AI-generated content

Generated by Breaking Change Documentation Tool - 2025-10-03 13:49:29

@tarekgh

Copy link
Copy Markdown
Member

@ericstj the breaking change for this is already submitted and finalized dotnet/docs#44282.

@tarekghtarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 3, 2025
@ericstj

Copy link
Copy Markdown
Member

Thank you @tarekgh

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Activitybreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.community-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@CodeBlanch@tarekgh@noahfalk@ericstj@alanwest@cijothomas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) - #111289

Merged
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix
Jan 14, 2025
Merged

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s)#111289
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix

Conversation

@CodeBlanch

Copy link
Copy Markdown
Contributor

Changes

  • Reset ActivityTraceFlags.Recorded when creating activity if not requested by sampler(s) because it may be inherited from a parent

Details

[Ran into this while working on #6058.]

Scenario:

  • We have an incoming request with traceparent marked as Recorded.
  • OTel sampler decides to drop for whatever reason.
  • Because this is a root, even though a drop was requested, OTel SDK will still create a propagation-only span/activity.
  • Here is the issue: The created span/activity inherits recorded from the parent context and therefore has Recorded=true. A propagation-only span should have Recorded=false.

/cc @tarekgh@noahfalk@samsp-msft@cijothomas@alanwest

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 10, 2025
@tarekghtarekgh added this to the 10.0.0 milestone Jan 10, 2025
@tarekgh

Copy link
Copy Markdown
Member
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?


Refers to: src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs:1236 in 5d2e54d. [](commit_id = 5d2e54d, deletion_comment = False)

@tarekgh
tarekgh requested a review from noahfalkJanuary 10, 2025 22:42
@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?

@tarekgh I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

Kind of hard to say.

Here are some thoughts...

The default sampler in OTel is ParentBased(AlwaysOn) which means first check the parent. If the parent is marked as Recorded (aka sampled) then respect that and create a recorded child. If there is no parent, create a recorded/sampled span (AlwaysOn).

So no one using the defaults will be impacted.

To be impacted users would have to switch the sampler or make a custom one which attempts to drop remote things. The bug is such that the drop isn't really respected. Recorded causes the spans to be exported (but they have IsAllDataRequested=false so they won't be populated). Impact is probably higher costs, maybe some screwy spans without data. What may be more interesting is that span, if it is propagated out of proc, will be transmitted as Recorded. So some other system downstream using ParentBased might decide to sample it fully essentially reversing the drop decision.

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

@tarekgh

Copy link
Copy Markdown
Member

I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

We don't have to remove the line if we are worried about the future adding to the flags. But we can just modify it to the following:

activity.ActivityTraceFlags=parentContext.TraceFlags&~ActivityTraceFlags.Recorded;

@tarekgh

Copy link
Copy Markdown
Member

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

@tarekghtarekgh 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.

@noahfalk

Copy link
Copy Markdown
Member

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

Yeah, I think a breaking change notice would be warranted here. For downlevel what is the plan? Just warn users that they won't be able to disable the recorded flag?

@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch could you please file the breaking change in https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml? Thanks!

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@tarekgh Done, see link above ^

@tarekghtarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jan 14, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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.

@dotnet-policy-servicedotnet-policy-serviceBot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 14, 2025
@tarekgh

Copy link
Copy Markdown
Member

/ba-g the failure is known build timeout issue happening in other PRs too.

@tarekgh
tarekgh merged commit 7132268 into dotnet:mainJan 14, 2025
Comment on lines -1236 to +1238
activity.ActivityTraceFlags = parentContext.TraceFlags;
// Note: Don't inherit Recorded from parent as it is set below
// based on sampling decision
activity.ActivityTraceFlags = parentContext.TraceFlags & ~ActivityTraceFlags.Recorded;

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.

Why does this line exist at all? If W3C trace context introduces new flags, what is to say that inheriting the flags from the parent will be the correct thing to do for those new flags?

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.

Ah, I see this was already discussed... removing the line seems more correct to me...

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.

In looking at the latest editor's draft of the spec, there is a new flag proposed: Random Trace ID Flag. My read of this flag is that it actually should be inherited from the parent, but there's no way to be certain this will be true for all new flags.

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.

Keeping this line is not hurting for now. Any newly introduced flag will need to be decided if it is inheritable from the parent or not anyway.

@ericstj

Copy link
Copy Markdown
Member

📋 Breaking Change Documentation Required

Create a breaking change issue with AI-generated content

Generated by Breaking Change Documentation Tool - 2025-10-03 13:49:29

@tarekgh

Copy link
Copy Markdown
Member

@ericstj the breaking change for this is already submitted and finalized dotnet/docs#44282.

@tarekghtarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 3, 2025
@ericstj

Copy link
Copy Markdown
Member

Thank you @tarekgh

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Activitybreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.community-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@CodeBlanch@tarekgh@noahfalk@ericstj@alanwest@cijothomas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) - #111289

Merged
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix
Jan 14, 2025
Merged

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s)#111289
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix

Conversation

@CodeBlanch

Copy link
Copy Markdown
Contributor

Changes

  • Reset ActivityTraceFlags.Recorded when creating activity if not requested by sampler(s) because it may be inherited from a parent

Details

[Ran into this while working on #6058.]

Scenario:

  • We have an incoming request with traceparent marked as Recorded.
  • OTel sampler decides to drop for whatever reason.
  • Because this is a root, even though a drop was requested, OTel SDK will still create a propagation-only span/activity.
  • Here is the issue: The created span/activity inherits recorded from the parent context and therefore has Recorded=true. A propagation-only span should have Recorded=false.

/cc @tarekgh@noahfalk@samsp-msft@cijothomas@alanwest

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 10, 2025
@tarekghtarekgh added this to the 10.0.0 milestone Jan 10, 2025
@tarekgh

Copy link
Copy Markdown
Member
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?


Refers to: src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs:1236 in 5d2e54d. [](commit_id = 5d2e54d, deletion_comment = False)

@tarekgh
tarekgh requested a review from noahfalkJanuary 10, 2025 22:42
@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?

@tarekgh I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

Kind of hard to say.

Here are some thoughts...

The default sampler in OTel is ParentBased(AlwaysOn) which means first check the parent. If the parent is marked as Recorded (aka sampled) then respect that and create a recorded child. If there is no parent, create a recorded/sampled span (AlwaysOn).

So no one using the defaults will be impacted.

To be impacted users would have to switch the sampler or make a custom one which attempts to drop remote things. The bug is such that the drop isn't really respected. Recorded causes the spans to be exported (but they have IsAllDataRequested=false so they won't be populated). Impact is probably higher costs, maybe some screwy spans without data. What may be more interesting is that span, if it is propagated out of proc, will be transmitted as Recorded. So some other system downstream using ParentBased might decide to sample it fully essentially reversing the drop decision.

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

@tarekgh

Copy link
Copy Markdown
Member

I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

We don't have to remove the line if we are worried about the future adding to the flags. But we can just modify it to the following:

activity.ActivityTraceFlags=parentContext.TraceFlags&~ActivityTraceFlags.Recorded;

@tarekgh

Copy link
Copy Markdown
Member

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

@tarekghtarekgh 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.

@noahfalk

Copy link
Copy Markdown
Member

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

Yeah, I think a breaking change notice would be warranted here. For downlevel what is the plan? Just warn users that they won't be able to disable the recorded flag?

@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch could you please file the breaking change in https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml? Thanks!

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@tarekgh Done, see link above ^

@tarekghtarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jan 14, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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.

@dotnet-policy-servicedotnet-policy-serviceBot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 14, 2025
@tarekgh

Copy link
Copy Markdown
Member

/ba-g the failure is known build timeout issue happening in other PRs too.

@tarekgh
tarekgh merged commit 7132268 into dotnet:mainJan 14, 2025
Comment on lines -1236 to +1238
activity.ActivityTraceFlags = parentContext.TraceFlags;
// Note: Don't inherit Recorded from parent as it is set below
// based on sampling decision
activity.ActivityTraceFlags = parentContext.TraceFlags & ~ActivityTraceFlags.Recorded;

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.

Why does this line exist at all? If W3C trace context introduces new flags, what is to say that inheriting the flags from the parent will be the correct thing to do for those new flags?

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.

Ah, I see this was already discussed... removing the line seems more correct to me...

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.

In looking at the latest editor's draft of the spec, there is a new flag proposed: Random Trace ID Flag. My read of this flag is that it actually should be inherited from the parent, but there's no way to be certain this will be true for all new flags.

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.

Keeping this line is not hurting for now. Any newly introduced flag will need to be decided if it is inheritable from the parent or not anyway.

@ericstj

Copy link
Copy Markdown
Member

📋 Breaking Change Documentation Required

Create a breaking change issue with AI-generated content

Generated by Breaking Change Documentation Tool - 2025-10-03 13:49:29

@tarekgh

Copy link
Copy Markdown
Member

@ericstj the breaking change for this is already submitted and finalized dotnet/docs#44282.

@tarekghtarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 3, 2025
@ericstj

Copy link
Copy Markdown
Member

Thank you @tarekgh

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Activitybreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.community-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@CodeBlanch@tarekgh@noahfalk@ericstj@alanwest@cijothomas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s) - #111289

Merged
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix
Jan 14, 2025
Merged

[System.Diagnostics.DiagnosticSource.Activity] Reset recorded flag when creating activity when not requested by sampler(s)#111289
tarekgh merged 4 commits into
dotnet:mainfrom
CodeBlanch:activity-sampling-fix

Conversation

@CodeBlanch

Copy link
Copy Markdown
Contributor

Changes

  • Reset ActivityTraceFlags.Recorded when creating activity if not requested by sampler(s) because it may be inherited from a parent

Details

[Ran into this while working on #6058.]

Scenario:

  • We have an incoming request with traceparent marked as Recorded.
  • OTel sampler decides to drop for whatever reason.
  • Because this is a root, even though a drop was requested, OTel SDK will still create a propagation-only span/activity.
  • Here is the issue: The created span/activity inherits recorded from the parent context and therefore has Recorded=true. A propagation-only span should have Recorded=false.

/cc @tarekgh@noahfalk@samsp-msft@cijothomas@alanwest

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Jan 10, 2025
@tarekghtarekgh added this to the 10.0.0 milestone Jan 10, 2025
@tarekgh

Copy link
Copy Markdown
Member
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?


Refers to: src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs:1236 in 5d2e54d. [](commit_id = 5d2e54d, deletion_comment = False)

@tarekgh
tarekgh requested a review from noahfalkJanuary 10, 2025 22:42
@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor
 activity.ActivityTraceFlags = parentContext.TraceFlags;

should the fix done here instead?

@tarekgh I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@CodeBlanch how likely this change can break anyone depending on the current behavior? I am asking to know if we need to file a breaking change for that.

Kind of hard to say.

Here are some thoughts...

The default sampler in OTel is ParentBased(AlwaysOn) which means first check the parent. If the parent is marked as Recorded (aka sampled) then respect that and create a recorded child. If there is no parent, create a recorded/sampled span (AlwaysOn).

So no one using the defaults will be impacted.

To be impacted users would have to switch the sampler or make a custom one which attempts to drop remote things. The bug is such that the drop isn't really respected. Recorded causes the spans to be exported (but they have IsAllDataRequested=false so they won't be populated). Impact is probably higher costs, maybe some screwy spans without data. What may be more interesting is that span, if it is propagated out of proc, will be transmitted as Recorded. So some other system downstream using ParentBased might decide to sample it fully essentially reversing the drop decision.

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

@tarekgh

Copy link
Copy Markdown
Member

I thought about just removing that line. That would probably take care of it. The reason I went with the explicit clear is because right now ActivityTraceFlags only has the Recorded definition. But who knows in the future it may have other flags which actually should inherit from the parent. Kind of hard to predict 😄 Happy to this though just LMK.

We don't have to remove the line if we are worried about the future adding to the flags. But we can just modify it to the following:

activity.ActivityTraceFlags=parentContext.TraceFlags&~ActivityTraceFlags.Recorded;

@tarekgh

Copy link
Copy Markdown
Member

FWIW if this was being done in OTel SDK we would probably call it a breaking change to align behavior with spec.

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

@tarekghtarekgh 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.

@noahfalk

Copy link
Copy Markdown
Member

That makes me feel we need to file a breaking change to get this documented. @noahfalk may weigh on that too. Here is where you can file the breaking change https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml.

Yeah, I think a breaking change notice would be warranted here. For downlevel what is the plan? Just warn users that they won't be able to disable the recorded flag?

@tarekgh

Copy link
Copy Markdown
Member

@CodeBlanch could you please file the breaking change in https://github.com/dotnet/docs/issues/new?template=02-breaking-change.yml? Thanks!

@CodeBlanch

Copy link
Copy Markdown
ContributorAuthor

@tarekgh Done, see link above ^

@tarekghtarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jan 14, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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.

@dotnet-policy-servicedotnet-policy-serviceBot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jan 14, 2025
@tarekgh

Copy link
Copy Markdown
Member

/ba-g the failure is known build timeout issue happening in other PRs too.

@tarekgh
tarekgh merged commit 7132268 into dotnet:mainJan 14, 2025
Comment on lines -1236 to +1238
activity.ActivityTraceFlags = parentContext.TraceFlags;
// Note: Don't inherit Recorded from parent as it is set below
// based on sampling decision
activity.ActivityTraceFlags = parentContext.TraceFlags & ~ActivityTraceFlags.Recorded;

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.

Why does this line exist at all? If W3C trace context introduces new flags, what is to say that inheriting the flags from the parent will be the correct thing to do for those new flags?

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.

Ah, I see this was already discussed... removing the line seems more correct to me...

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.

In looking at the latest editor's draft of the spec, there is a new flag proposed: Random Trace ID Flag. My read of this flag is that it actually should be inherited from the parent, but there's no way to be certain this will be true for all new flags.

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.

Keeping this line is not hurting for now. Any newly introduced flag will need to be decided if it is inheritable from the parent or not anyway.

@ericstj

Copy link
Copy Markdown
Member

📋 Breaking Change Documentation Required

Create a breaking change issue with AI-generated content

Generated by Breaking Change Documentation Tool - 2025-10-03 13:49:29

@tarekgh

Copy link
Copy Markdown
Member

@ericstj the breaking change for this is already submitted and finalized dotnet/docs#44282.

@tarekghtarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 3, 2025
@ericstj

Copy link
Copy Markdown
Member

Thank you @tarekgh

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Diagnostics.Activitybreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.community-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@CodeBlanch@tarekgh@noahfalk@ericstj@alanwest@cijothomas