Skip to content

Avoid copying Activity tags and links when sampling - #135277

Open
martincostello wants to merge 3 commits into
dotnet:mainfrom
martincostello:gh-135007
Open

martincostello wants to merge 3 commits into
dotnet:mainfrom
martincostello:gh-135007

Conversation

@martincostello

Copy link
Copy Markdown
Member

Do not copy links and tags when creating a new Activity when ActivitySamplingResult.PropagationData is specified.

Fixes #135007.

Benchmarks

BenchmarkDotNet, default job, --inProcess --affinity 4095, creating, starting and stopping an activity with the listener returning the given sampling result. main vs. PR; the tag set is the 9 tags ASP.NET Core-style server instrumentation passes at creation, plus one link. Ratios are PR / main (lower is better).

Sampling Creation data Method main Mean main Allocated PR Mean PR Allocated Time Ratio Alloc Ratio
PropagationData None NoTags 120.8 ns 416 B 122.0 ns 416 B 1.01 1.00
PropagationData 9 tags NineTags 211.2 ns 848 B 116.0 ns 416 B 0.55 0.49
PropagationData 9 tags + 1 link NineTagsAndLink 234.7 ns 976 B 116.0 ns 416 B 0.49 0.43
AllDataAndRecorded None NoTags 126.7 ns 416 B 116.4 ns 416 B 0.92 1.00
AllDataAndRecorded 9 tags NineTags 202.0 ns 848 B 195.0 ns 848 B 0.97 1.00
AllDataAndRecorded 9 tags + 1 link NineTagsAndLink 242.9 ns 976 B 225.4 ns 976 B 0.93 1.00
Benchmark Code
using System.Diagnostics;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(PropagationDataBenchmarks).Assembly).Run(args);

[MemoryDiagnoser]
public class PropagationDataBenchmarks
{
    private static readonly KeyValuePair<string, object?>[] s_tags =
    [
        new("client.address", "192.0.2.1"), new("network.peer.address", "192.0.2.1"), new("network.peer.port", 50000),
        new("server.address", "localhost"), new("server.port", 5000), new("http.request.method", "GET"),
        new("user_agent.original", "Mozilla/5.0"), new("url.scheme", "https"), new("url.path", "/api/items"),
    ];

    private static readonly ActivityLink[] s_links = [new(new ActivityContext(ActivityTraceId.CreateRandom(), ActivitySpanId.CreateRandom(), ActivityTraceFlags.None))];

    private ActivitySource _source = null!;
    private ActivityListener _listener = null!;
    private ActivitySamplingResult _result;

    [Params(ActivitySamplingResult.PropagationData, ActivitySamplingResult.AllDataAndRecorded)]
    public ActivitySamplingResult Sampling { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        Console.WriteLine("Assembly: " + typeof(Activity).Assembly.Location);
        _result = Sampling;
        _source = new ActivitySource("Bench");
        _listener = new ActivityListener
        {
            ShouldListenTo = s => ReferenceEquals(s, _source),
            Sample = (ref _) => _result,
        };
        ActivitySource.AddActivityListener(_listener);
    }

    [GlobalCleanup]
    public void Cleanup() { _listener.Dispose(); _source.Dispose(); }

    [Benchmark(Baseline = true)]
    public void NoTags() => Run(null, null);

    [Benchmark]
    public void NineTags() => Run(s_tags, null);

    [Benchmark]
    public void NineTagsAndLink() => Run(s_tags, s_links);

    private void Run(KeyValuePair<string, object?>[]? tags, ActivityLink[]? links)
    {
        var a = _source.CreateActivity("Request", ActivityKind.Server, default(ActivityContext), tags, links);
        a!.Start();
        a.Stop();
    }
}

Do not copy links and tags when creating a new `Activity` when `ActivitySamplingResult.PropagationData` is specified.

Fixes dotnet#135007.
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Oct 6, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Sampler tags violate their documented contract, and the behavioral change needs compatibility documentation.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
What changed in this PR

Optimizes Activity creation for PropagationData sampling.

Changes:

  • Skips copying creation tags, links, and sampler tags.
  • Adds coverage for parent formats and multiple listeners.
File Description
Activity.cs Conditionally skips data copying.
ActivitySourceTests.cs Tests the revised sampling behavior.

Comment on lines +1200 to +1201
// Links and tags are unnecessary for activities sampled as PropagationData, so skip copying them.
bool copyData = request != ActivitySamplingResult.PropagationData;

@tarekgh tarekgh Oct 6, 2026 •

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.

@martincostello The SamplingTags documentation promises that tags added during sampling are added to a created activity. Could we leave if (samplerTags != null) ungated and update the test to expect those tags under PropagationData?

This would not address the separate compatibility concern for creation tags and links. We should weigh that observable change against the measured benefit and document it if we proceed.

@tarekgh tarekgh added this to the 12.0.0 milestone Oct 6, 2026
- Harden check for skipping tags.
- Don't skip sampler tags.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Public API documentation remains inaccurate for the new sampling semantics.

Review effort: Balanced
Findings: 2 Low severity

Open (2)

Comment on lines +1200 to +1201
// Links and creation tags are unnecessary for activities sampled as PropagationData, so skip copying them.
bool copyData = request is ActivitySamplingResult.AllData or ActivitySamplingResult.AllDataAndRecorded;

@tarekgh tarekgh Oct 6, 2026 •

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.

The documentation concern is valid. Here is suggested replacement wording for the tags and links parameter documentation in ActivitySource.cs, to apply consistently to the two CreateActivity overloads and three StartActivity overloads that accept those parameters.

        /// <param name="tags">
        /// The optional tags made available to sampling callbacks and copied to the created activity
        /// only when the combined sampling result is <see cref="ActivitySamplingResult.AllData"/>
        /// or <see cref="ActivitySamplingResult.AllDataAndRecorded"/>.
        /// </param>
        /// <param name="links">
        /// The optional links made available to sampling callbacks and copied to the created activity
        /// only when the combined sampling result is <see cref="ActivitySamplingResult.AllData"/>
        /// or <see cref="ActivitySamplingResult.AllDataAndRecorded"/>.
        /// </param>

This wording applies only to caller-supplied creation tags and links. Tags added through ActivityCreationOptions.SamplingTags are still copied when the combined result is PropagationData.

The block above is for manual application to ActivitySource.cs, not to the Activity.cs lines attached to this thread. ActivitySource.cs is not currently part of the PR diff, so this is not a directly applicable suggestion here.

Update `ActivitySource,CreateActivity()` parameter documentation for `tags` and `links` for new behaviour.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The existing unresolved compatibility-documentation requirement blocks approval despite the implementation and tests appearing sound.

Review effort: Balanced
Findings: 2 Low severity

Open (2)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Diagnostics.Activity community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ActivitySource copies creation tags and links into activities sampled as PropagationData

3 participants