Duplicated activity baggage when calling SetBaggage on parent and current activity

[AndreyTretyak]

I'm using System.Diagnostics.DiagnosticSource with version 6.0.0-rc.1.21451.13 for project that targets net5.0.

If I understand correctly SetBaggage suppose to replace value of the baggage with the same key if it's present. And this is also stated in the docs.

But when I'm trying to call SetBaggage for the key that was present in the baggage of parent activity it added as duplicated item.

For example, following code:

using System;
using System.Diagnostics;

using (var activity = new Activity("A")
    .Start()
    .SetBaggage("Test", "TA"))
{
    using (var subActivity = new Activity("B")
        .Start()
        .SetBaggage("Test", "TB"))
    {
        foreach (var pair in subActivity.Baggage)
        {
            Console.WriteLine("{0}:{1}", pair.Key, pair.Value);
        }
    }
}

Will output:

Test:TB
Test:TA

Probably more common case, is setting baggage in ActivityListener.ActivityStarted handler:

using System;
using System.Diagnostics;

var listener = new ActivityListener();
listener.ActivityStarted += a => { a.SetBaggage("Test", a.OperationName); };
listener.Sample += (ref ActivityCreationOptions<ActivityContext> context) => ActivitySamplingResult.AllData;
listener.SampleUsingParentId += (ref ActivityCreationOptions<string> context) => ActivitySamplingResult.AllData;
listener.ShouldListenTo += source => true;
ActivitySource.AddActivityListener(listener);

var source = new ActivitySource("TestSource");
using (var activity = source.StartActivity("SA"))
{
    using (var subActivity = source.StartActivity("SB"))
    {
        foreach (var pair in subActivity.Baggage)
        {
            Console.WriteLine("{0}:{1}", pair.Key, pair.Value);
        }
    }
}

It will output:

Test:SB
Test:SA

If this behavior is intentional, to avoid overriding parent baggage or due to implementation details, then at least docs should be updated, but I think it would be much better if behavior could be changed to override value with the same key as SetBaggage suppose to work, since current approach could cause a lot of confusions and situation when baggage grow more then it should.

[ghost]

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

Issue Details

---

I'm using `System.Diagnostics.DiagnosticSource` with version `6.0.0-rc.1.21451.13` for project that targets `net5.0`. If I understand correctly `SetBaggage` suppose to replace value of the baggage with the same key if it's present. And this is also stated in the [docs](https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.activity.setbaggage?view=net-6.0#System_Diagnostics_Activity_SetBaggage_System_String_System_String_ ). But when I'm trying to call `SetBaggage` for the key that was present in the baggage of parent activity it added as duplicated item. For example, following code: ```CSharp using System; using System.Diagnostics; using (var activity = new Activity("A") .Start() .SetBaggage("Test", "TA")) { using (var subActivity = new Activity("B") .Start() .SetBaggage("Test", "TB")) { foreach (var pair in subActivity.Baggage) { Console.WriteLine("{0}:{1}", pair.Key, pair.Value); } } } ``` Will output: ``` Test:TB Test:TA ``` Probably more common case, is setting baggage in `ActivityListener.ActivityStarted` handler: ```CSharp using System; using System.Diagnostics; var listener = new ActivityListener(); listener.ActivityStarted += a => { a.SetBaggage("Test", a.OperationName); }; listener.Sample += (ref ActivityCreationOptions context) => ActivitySamplingResult.AllData; listener.SampleUsingParentId += (ref ActivityCreationOptions context) => ActivitySamplingResult.AllData; listener.ShouldListenTo += source => true; ActivitySource.AddActivityListener(listener); var source = new ActivitySource("TestSource"); using (var activity = source.StartActivity("SA")) { using (var subActivity = source.StartActivity("SB")) { foreach (var pair in subActivity.Baggage) { Console.WriteLine("{0}:{1}", pair.Key, pair.Value); } } } ``` It will output: ``` Test:SB Test:SA ``` If this behavior is intentional, to avoid overriding parent baggage or due to implementation details, then at least docs should be updated, but I think it would be much better if behavior could be changed to override value with the same key as `SetBaggage` suppose to work, since this could cause a lot of confusions and situation when baggage grow more then it should.

Author:	AndreyTretyak
Assignees:	-
Labels:	`untriaged`, `area-System.Diagnostics.Activity`
Milestone:	-

[tarekgh]

The current behavior of SetBaggage is not allowing the duplication only on the Activity instance (not the whole parent chain). If we need to change this behavior, that will be a breaking change as some applications may be depending on the current behavior.

CC @noahfalk

[AndreyTretyak]

From my point of view, it would make sense to change this behavior before SetBaggage was released with net6.0, but maybe I'm again too late with this suggestion.

In my mind, SetBaggage suppose to provide you with the way of overriding baggage item in current activity, irrelevant to its source (although I understand it would make implementation more complex), for example:

using System;
using System.Diagnostics;

using (var activity = new Activity("A")
    .Start()
    .SetBaggage("Test", "TA"))
{
    using (var subActivity = new Activity("B")
        .Start()
        .SetBaggage("Test", "TB"))  { }
}
// For activity Test baggage item should be equal to "TA"
// For subActivity Test baggage item should be equal to "TB"

Following behavior would provide the best control over baggage and will allow fully prevent cases uncontrollable baggage grows, that could happen now if you set some baggage for each activity, for example in StartActivity handler.

[tarekgh]

From my point of view, it would make sense to change this behavior before SetBaggage was released with net6.0, but maybe I'm again too late with this suggestion.

Yes, it is too late to do so now considering the risk we may have of changing the code and behavior in the current stage. Think about other scenarios we may run into. For example, what happen if someone set the baggage on the parent, should that reflect on the child too? something like:

using (var activity = new Activity("A")
    .Start()
{
    using (var subActivity = new Activity("B")
        .Start()
        .SetBaggage("Test", "TB"))  
        { 
            activity.SetBaggage("Test", "TA"))
        }
}

What you expect to happen at that time?

In general, I am seeing we need to think more about that and decide in the future release what would be the best behavior to provide. We may consider a breaking change if needed to.

CC @shirhatti

[AndreyTretyak]

I would say that in a scenario that you've described subActivity should still have value TB and in parent activity it should have value TA. So we would have baggage item value set using SetBaggage in current activity overriding values inherited from parent activity.

It looks to me like the most straightforward behavior from user's point of view.

I assume merging of the lists could be done when the user requesting baggage, but then we would need to keep track of what items were added using SetBaggage instead of AddBaggage.

If this behavior won't be changed then maybe at least docs could be updated to clarify that there is no way of updating tags that you've inherited from the parent activity. And if we are talking about the change in 7.0.0 could the behavior of SetBaggage be changed or it would have to be another overload with a flag like overrideValueFromParent? I'm worried that we would have too many ways of adding baggage.

[tarekgh]

I would say that in a scenario that you've described subActivity should still have value TB and in parent activity it should have value TA. So we would have baggage item value set using SetBaggage in current activity overriding values inherited from parent activity. It looks to me like the most straightforward behavior from user's point of view.

The confusion would be depending on the order of the calls will get different results. I am not pushing back on any idea here more than just telling this need to be carefully thought in before we do it.

And if we are talking about the change in 7.0.0 could the behavior of SetBaggage be changed or it would have to be another overload with a flag like overrideValueFromParent? I'm worried that we would have too many ways of adding baggage.

I am not going to vote for having extra overload here. I would say we can have the breaking change and we can mitigate this breaking with some config switch if we think scope of the change can break many customers. But as I said we need to think more about the whole thing. Thanks for raising this issue.

[tarekgh]

If this behavior won't be changed then maybe at least docs could be updated to clarify that there is no way of updating tags that you've inherited from the parent activity.

I forgot to mention this is a good idea. Do you want to submit a PR to add this comment to the doc https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.activity.setbaggage?view=net-6.0#System_Diagnostics_Activity_SetBaggage_System_String_System_String_?

[AndreyTretyak]

The confusion would be depending on the order of the calls will get different results. I am not pushing back on any idea here more than just telling this need to be carefully thought in before we do it.

In the solution that I'm trying to describe result enumeration of the baggage items would be created at the moment when the user requests it. It would be done by combining baggage from the current activity and its parent. So there won't be a dependency between the order of calls on parent and current activity they would have an independent list the would be combined in the way that baggage item in current would take precedence (if it's set using SetBaggage) and override item with the same key from a parent.

I understand it would make implementation more complicated, that's just my understanding of what behavior I would expect looking at the method and what would provide the most control to the user without adding confusion (the list would look seamless and editable to the user on each activity). I'm open to discussing other approaches.

[tarekgh]

@AndreyTretyak Just to double check, this issue is not a blocker to you to use .NET 6.0 as there is a way to manually filter the list if needed. Right?

[AndreyTretyak]

Not sure yet to be honest. I've just updated to RC package to use Metrics API and notices failure due to UT failures.

First of all does current implementation guaranty that value set in the current activity would be on the list before value with the same key from parent? So we can use it to distinguish between them?

I would also need to check how our trace exporters handle duplicated baggage since we do not own them and there are no way of updating Activity to exclude duplicate.

Another aspect I'm worried about is that current behavior increate the size of the baggage so with enough nesting it could become bigger then could be transferred in http header (or in any other way) and will block the request, because again there are no way of removing duplicates from current activity.

[tarekgh]

First of all does current implementation guaranty that value set in the current activity would be on the list before value with the same key from parent? So we can use it to distinguish between them?

Yes, we always enumerate the child's baggage before the parent. https://github.com/dotnet/runtime/blob/44b44501c76c46bd79ee52b7d9a9d8a4957fc85f/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L327

Another aspect I'm worried about is that current behavior increate the size of the baggage so with enough nesting it could become bigger then could be transferred in http header (or in any other way) and will block the request, because again there are no way of removing duplicates from current activity.

I am not sure why current behavior do that. Wouldn't that be the behavior before introducing such APIs? or you had a different logic before using SetBaggage API?

I believe the best work around for you is when calling SetBaggage, remove the Baggage with the same key from the parent. do something like:

currentActivity.SetBaggage(key, value);

for (Activity? activity = currentActivity.Parent; activity != null; activity = activity.Parent)
{
    activity.SetBaggage(key, null);
}

[AndreyTretyak]

Thank you for confirming the order.

To solve the growth problem while waiting for SetBaggage we had to use a hack involving reflection to replace value if it was present. After looking into that hack I think it was working similar to the current SetBaggage (replacing value only for current activity) so I think we are mostly fine and should not be blocked.

I guess the scenario of deep nesting, which will make baggage bigger then http header allows, is theoretical for us. I understand that it is complicated case without an easy fix. The solution that I was suggesting initially does not solve size growth as well.

I think we'll try using SetBaggage directly for now, since it allows us to keep separate values for each activity in a chain. And if we hit issues with the size growing we'll try using the workaround that you've suggested.

Thank you for helping me understand to understand the situation.

[AndreyTretyak]

If this behavior won't be changed then maybe at least docs could be updated to clarify that there is no way of updating tags that you've inherited from the parent activity.

I forgot to mention this is a good idea. Do you want to submit a PR to add this comment to the doc https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.activity.setbaggage?view=net-6.0#System_Diagnostics_Activity_SetBaggage_System_String_System_String_?

I've created PR to update docs https://github.com/dotnet/dotnet-api-docs/pull/7217 Feel free to suggest better wording.