Add ability to update Activity.Baggage

[AndreyTretyak]

Background and Motivation

Follow up after #42596

Currently, there is no way of updating or removing the existing values of Activity.Baggage. The only method available to change Baggage state is AddBaggage and calling it twice with the same key would add two items to the list. It's important to note that Baggage should be kept as small as possible because it's intended to be serialized over the wire across the process/machine.

I could see a couple of cases when an update is needed:

1. Unintended calls of the same AddBaggage multiple times that won't add any useful information but will increase the size of the Baggage. For example, your services have a middleware that adds some value to Baggage, on each service in the call chain you would get one more identical item added, and eventually, it could surpass the allowed size of the header that used to transfer Baggage via web requests. This case could be avoided by checking that item already present (not very elegant with current API), but only if you have access to modify code that adds items.

2. Third-party code adding unwanted items to Baggage that you would like to remove to avoid transferring them or if they control some logic.

3. The most annoying case is when you need to update the value for an existing item. After calling AddBaggage with the same key GetBaggageItem would return the updated value, but your baggage size is doubled, add in addition to that during transition over the wire order of the baggage items could be changed (since they need to be added in reverse order to preserve it, for example reordering currently happening in asp .net core), that would result in GetBaggageItem returning different values depending on the amount of transfers baggage had. So having items with identical keys in the Baggage is a highly undesired state, but currently, there is no way to avoid it if you need a value update.

To add to that described behaviors easy to miss assuming dictionary-like structure behind AddBaggage/GetBaggageItem since they mostly behave the same before you look inside Baggage collection or will try to transfer it.

Proposed API

namespace System.Diagnostics
{
    public partial class Activity : IDisposable
    {
        public Activity SetBaggage(string key, string? value)
    }
}

All of the mentioned problems could be solved by adding SetBaggage method analogous to Activity.SetTag method. Alternative naming could be SetBaggageItem, but it's more important to make it consistent with AddBaggage then with GetBaggageItem, and it's shorter :)

It might be initially confusing for users to have both AddBaggage and SetBaggage methods, but it should motivate to look more attentively in the behavior of AddBaggage to prevent them from having duplicate keys.

Similar to SetTag, passing null as value parameter into SetBaggage would remove item. That is especially important since Baggage should be kept as small as possible.

An interesting case here is that Baggage allows you to have multiple items with the same key. So there are multiple possible behaviours in this case.

Removal/update of the first matched item makes the most sense. It would be the item that is returned by GetBaggageItem, so we would have those two methods matched. Beside that, having multiple items with the same key should be a very rare case (especially with this API available), almost always it would be a single item so all approaches would have the same end result, and removing the first matched is the most straightforward from the implementation point of view plus it could be done without iterating through the whole list.

Alternative options could be removal/update of the last element that could lead to very confusing behavior when combined with GetBaggageItem that returns first element. Or removal/update of all occurrences that whould add overhead on each call, to cover a really narrow use case plus setting all items to the same value does not have a lot of practical usage.

Usage Examples

SetBaggage could be used instead of AddBaggage in cases when it's possible that code might be executed twice in the call chain (for example middleware or some infrastructure code) to guaranty that Baggage size won't be expanded unintentionally and contains a single entry for each key.

The ability to remove/update items in the Baggage also would be used to remove unneeded items added by a third party and for quick hacks to check that baggage item presence/absence/spesial value provides the desired result or creates bug.

Method have similar API to existing Activity methods and could be chained with others.

Risks

SetBaggage would be slower than AddBaggage (that it will replace in some scenarios) since instead of just adding an item it will need to iterate over the Baggage list, but considering usually small size of Baggage performance degradation should not be significant, and presence of both methods would allow users to chose the most appropriate for their scenario.

[ghost]

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

[tarekgh]

Thanks @AndreyTretyak for logging this issue.

I would suggest to modify the proposal to behave as Activity.SetTag. So we'll not need to expose RemoveBaggageItem as the proposed 'SetBaggageItem' can remove the item if the input value is null. The benefit of this, we'll have a consistent design between Tags and Baggage. what you think?

CC @noahfalk

[AndreyTretyak]

Yes, sure, that would be even better.
Sorry, I'm quite new to the Activity API and have not noticed that functionality.
Will SetBaggageItem("key1", null) remove the first item with the key key1 in the Baggage or all of them?

[tarekgh]

Sorry, I'm quite new to the Activity API and have not noticed that functionality.

No problem at all. getting fresh opinions like yours help.

Will SetBaggageItem("key1", null) remove the first item with the key key1 in the Baggage or all of them?

The SetTag behavior is deleting the first item in the list. Note that Tags ordered as the items get added. For SetBaggageItem we can decide about the behavior. the options are:

• Remove the first matched item in the list. which will be the last entered item with that key.
• Remove the last matched item in the list. which will be the first entered item with that key.
• Remove all occurrences.

I am preferring the first option but I am open to discuss the other options.

One other point, the API should be called SetBaggage instead of SetBaggageItem. I prefer consistency with AddBaggage more than with GetBaggageItem. anyway this naming details we can discuss more when we review it with the design committee.

[AndreyTretyak]

I also think that the removal of the first matched item makes the most sense. First of all, it would be the item that is returned by GetBaggageItem, so we would have those two methods matched. Secondly, that is the first thing you expect on seeing this method. Lastly, I really hope that having multiple items with the same key would be a very rare case (especially with this API available), almost always it would be a single item so all approaches would have the same end result, and removing the first matched is the most straightforward from the implementation point of view plus it could be done without iterating through the whole list.

Removing the last element could lead to very confusing behavior when combined with GetBaggageItem.

Removing of all occurrences adds overhead on each call to cover a really narrow use case and it's a bit inconsistent with the behavior of SetBaggage when it's called with actual value.

I like the name SetBaggage more, it makes sense to match it with AddBaggage (totally missed that it does not have 'Item' in the name) and it's shorter.

[tarekgh]

@AndreyTretyak sounds good. could you please update the proposal on the top to reflect that?

[AndreyTretyak]

@tarekgh Updated, please have a look. Feel free to point out parts that need to be removed/updated if it looks too verbose.

[tarekgh]

@noahfalk the proposal looks good to me. please let me know if you have any comments so we can proceed with this proposal.

[noahfalk]

This looks very reasonable to me : )

[terrajobst]

Video

• Looks good as proposed

namespace System.Diagnostics
{
    public partial class Activity
    {
        public Activity SetBaggage(string key, string? value)
    }
}