Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking change]: Activity.Recorded behavior change for PropagationData sampling result with Recorded parent #44282

Open
1 of 3 tasks
CodeBlanch opened this issue Jan 14, 2025 · 0 comments
Assignees
Labels
breaking-change Indicates a .NET Core breaking change 🏁 Release: .NET 10 Work items for the .NET 10 release doc-idea Indicates issues that are suggestions for new topics [org][type][category] Pri1 High priority, do before Pri2 and Pri3 ⌚ Not Triaged Not triaged

Comments

@CodeBlanch
Copy link
Contributor

Description

The ActivitySource.CreateActivity and ActivitySource.StartActivity APIs only return an Activity when there is a registered listener which decides the instance should be created. This is generally known as sampling.

The ActivitySamplingResult enum defines the possible sampling decisions.

When creating an Activity without a parent, ActivitySamplingResult drives whether the Activity is created and then how the Recorded and IsAllDataRequested properties are set:

ActivitySamplingResult Activity created Activity.Recorded Activity.IsAllDataRequested
None No
PropagationData Yes False False
AllData Yes False True
AllDataAndRecorded Yes True True

It is also possible to create an Activity with a parent. The parent could be in the same process, or it could be a remote parent propagated to the current process.

An issue was discovered where if the parent has Recorded=true AND the ActivitySamplingResult is PropagationData then the created Activity has a Recorded value which breaks with the above:

ActivitySamplingResult Activity created Activity.Recorded Activity.IsAllDataRequested
PropagationData Yes True False

This may cause issues for users:

  • OpenTelemetry .NET, for example, will capture/export any Activity with Recorded=true. An Activity sampled as PropagationData should not be recorded by definition.

  • When performing external operations (such as HTTP calls) the current Activity is propagated to downstream systems. A PropagationData Activity should be propagated, but the current design causes it to be propagated as Recorded which may lead downstream systems to treat the trace as sampled.

dotnet/runtime#111289 adjusts the logic as such:

ActivitySamplingResult Activity created Activity.Recorded Activity.IsAllDataRequested
PropagationData Yes False False

This is a breaking change but one that corrects the behavior following the OpenTelemetry Specification.

Version

.NET 10 Preview 1

Previous behavior

When creating an Activity as PropagationData with a parent marked as Recorded:

ActivitySamplingResult Activity created Activity.Recorded Activity.IsAllDataRequested
PropagationData Yes True False

New behavior

When creating an Activity as PropagationData with a parent marked as Recorded:

ActivitySamplingResult Activity created Activity.Recorded Activity.IsAllDataRequested
PropagationData Yes False False

Type of breaking change

  • Binary incompatible: Existing binaries might encounter a breaking change in behavior, such as failure to load or execute, and if so, require recompilation.
  • Source incompatible: When recompiled using the new SDK or component or to target the new runtime, existing source code might require source changes to compile successfully.
  • Behavioral change: Existing binaries might behave differently at run time.

Reason for change

The existing behavior does not follow the OpenTelemetry Specification.

Recommended action

Users who have implemented ActivityListener.Sample directly AND use ActivitySamplingResult.PropagationData should verify they are not reliant on the flawed behavior. Activity.ActivityTraceFlags may be set to Recorded after the CreateActivity or StartActivity call to restore the previous behavior.

Users using OpenTelemetry .NET should verify their sampler configuration. The default OpenTelemetry .NET configuration uses a parent-based algorithm which is not impacted. Only users who have customized the sampler should verify the behavior.

Feature area

Other (please put exact area in description textbox)

Affected APIs

  • ActivitySource.CreateActivity
  • ActivitySource.StartActivity
@CodeBlanch CodeBlanch added breaking-change Indicates a .NET Core breaking change doc-idea Indicates issues that are suggestions for new topics [org][type][category] Pri1 High priority, do before Pri2 and Pri3 labels Jan 14, 2025
@dotnet-policy-service dotnet-policy-service bot added the ⌚ Not Triaged Not triaged label Jan 14, 2025
@gewarren gewarren added the 🏁 Release: .NET 10 Work items for the .NET 10 release label Jan 14, 2025
@dotnetrepoman dotnetrepoman bot added the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 16, 2025
@dotnet-policy-service dotnet-policy-service bot removed the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates a .NET Core breaking change 🏁 Release: .NET 10 Work items for the .NET 10 release doc-idea Indicates issues that are suggestions for new topics [org][type][category] Pri1 High priority, do before Pri2 and Pri3 ⌚ Not Triaged Not triaged
Projects
Development

No branches or pull requests

3 participants