Runtime: Add Activity.GetTagItem

[kzu]

Background and Motivation

Both Activity.Tags and Activity.Baggage have a somewhat unusual IEnumerable<KeyValuePair<string, string?>> type, which makes it cumbersome to access individual values (i.e. can't do activity.Tags.TryGetValue(key, out var value)). The latter adds a Activity.GetBaggageItem to compensate, but there isn't an equivalent one for Tags.

Proposed API

namespace System.Diagnostics
{
    public class Activity {
+        public string? GetTagItem(string key)

Usage Examples

``` C#
Activity activity = ...;

var tag = activity.GetTagItem("Foo");
if (tag != null) {
// do something with tag
}

## Alternative Designs

The obvious alternative would be to use the dictionary-style `bool TryGetTag(string key)` instead. 

```diff
namespace System.Diagnostics
{
    public class Activity {
+        public bool TryGetTag(string key, out string? value)

For consistency, that would likely require changing the existing GetBaggageItem(string key) to bool TryGetBaggage(string key) too, I think.

namespace System.Diagnostics
{
    public class Activity {
-        public string? GetBaggageItem(string key)
+        public bool TryGetBaggage(string key, out string? value)

I think this alternative proposal is much more idiomatic than the existing GetBaggageItem and the proposed GetTagItem.

Risks

None I can think of.

[msftbot[bot]]

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

[tarekgh]

@kzu Activity mainly collecting tags/Baggages and then when logging it will just get the list of these. could you please elaborate more on your scenario and what you are trying to do?

[tarekgh]

CC @noahfalk

[SingleAccretion]

@tarekgh This could be achieved if the new TagObjects property was exposed as IReadOnlyDictionary<string, object> instead of IEnumerable<string, object>. It is would be backed by ActivityTagsCollection which implements IDictionary<string, object> anyway.

That said, I feel like I am forgetting something from the review why was it not done this way.

[tarekgh]

@SingleAccretion we are exposing the list as IEnumerable to ensure the order in the list. The internal implementation is not using ActivityTagsCollection either because there is some behavior specific to Activity. e.g. AddTag(string, object) can allow adding a duplicated entry (with same key). ActivityTagsCollection wouldn't allow duplication. Alos, if we exposed it as ActivityTagsCollection that will make confusion to the API users to know how they can add tags by AddTag or by retrieving the ActivityTagsCollection and adding to it.

[SingleAccretion]

@tarekgh I see, then this would really be better exposed as a method on Activity itself. The problem, as I can guess, is that it effectively makes O(1) tag retrieval a part of the public contract which is not desirable.

[tarekgh]

@SingleAccretion could you please tell more about your scenario and what you are trying to do? https://github.com/dotnet/runtime/issues/39866#issuecomment-663231606

[SingleAccretion]

@tarekgh Well, I am not the original poster, I am just commenting on the API, so I am afraid I would not be of much help in coming up with (real-world) scenarios for it.

[tarekgh]

As I commented this is not the common scenario as Activity been there for years and this was not requested. Also, internally we are maintaining the list in order too. So even we expose it most likely wouldn't be O(1) except if we add overhead on the perf to do it O(1) and in same time be ordered which no-one so far required it.

[kzu]

The scenario is exactly the same as for GetBaggageItem @tarekgh. Whichever was the scenario that caused that API to be introduced (i.e. being able to read a baggage or tag item by key), is the same for this.

It's a usability improvement when retrieving items. So rather than having to do the foreach at the call site (something GetBaggageItem currently does precisely for this reason, I'd assume), encapsulate it in a TryGetTag(key) or GetTagItem), again, exactly like GetBaggageItem).

It will have the same O-ness as the existing GetBaggageItem, FWIW. This is not about requesting any performance caracteristics, just a more usable API for accessing what otherwise looks like a fairly unusual calling style for a dictionary-like bunch of key/values.

[kzu]

FWIW, my concrete scenario is that when ultimately logging from an ActivityListener, I need to check for the presence of certain tags to customize said logging/telemetry, and it was a bit surprising that there was no equivalent for Tags to what's there for Baggage, that's all.

[tarekgh]

@kzu sorry I am still not clear about the scenario. As I mentioned before, usually users of Activity just store tags/baggages and never bother reading it back. if there is logging or trace exporters, usually they get the whole list of the and then log it. So, I want to know your scenario require you to get specific Tag from the Activity object to do something with it. I am asking because Activity exists for many years and all API users never asked for something like this. what kind of customization you are doing which need to do it according to the tags/baggages? I am not pushing back more than trying to understand the need.

[kzu]

Would you care to explain why there is an asymetric API where you have GetBaggageItem but not a GetTagItem? That's unusual, I'd say, and moreover might give the impression that somehow Baggage deserves different treatment? As an API user, I see one and not the other... should I infer that Tags are not intended to be retrieved by key but Baggage is, even though both properties have the same underlying type?

Even for the sake of polishment I think it makes sense, given one property already has a friendlier accessor and not the other.

(not that anyone can't trivially "solve" this with an extension method, mind you, but it's weird to have GetBaggageItem there then, if you can't think of an explanation/scenario that drove that member in the first place 😉. Do note that in your responses, you're referring to tags/baggges interchangeably, yet the API is not symetric for both, so what's special about Baggage property?).

[tarekgh]

@kzu to clarify, I am not pushing back on your request more than trying to understand your scenario.

Although I don't know the whole history of the Activity class and why each API is added there, this doesn't mean we should proactively add APIs when it is not clear if the users will benefit from it. Even you see Baggage is not symmetric with Tags, doesn't mean we should just add new API for only to be symmetric or to just to polish it. Adding API has a cost and we should be sure it is really needed.

So going back to my original question, could you please send more details about the scenario you need to have the proposed API? This can help a lot to know if this is important to have it and when. Thanks for your feedback.

[kzu]

My scenario is to add a tag only if it's not there already. In a cross-cutting piece of behavior in a micro-services app running in azure functions (think middleware).

[tarekgh]

thanks @kzu.

in your scenario, are you trying to avoid adding 2 tags with the same key? or you are trying to not overriding the old value of the tag?

[kzu]

avoid adding 2 tags/baggages. For the latter, I can already get it and check for nulls. For tags, I foreach. I just made myself an extension method just like the one proposed here, so I have the same code for both actually (since I do some tags and some baggages, depending on whether I want them propagated or not).

[tarekgh]

for tags you can use SetTag(...) This will not add extra tag. if the tag exists it will just update its value. if the tag not exist it'll get added.

[kzu]

So, I guess since I couldn't make it obvious that the API is far from obvious in its current form, adding the missing method will likely never happen :(. I say missing because I haven't heard a single argument against adding the method that wouldn't equally apply to the existing GetBaggageItem...

[tarekgh]

adding the missing method will likely never happen

I am not sure why you have jumped to the conclusion. I see this is nice to have but it is not blocking anyone so far. So, considering it in the next releases is possible.

[tarekgh]

@kzu please let me know if you are ok to have this form the next release and nothing is blocking you for now.

[kzu]

I see this is nice to have

First time I get this mpression from you, that's all. All along it felt like I couldn't make a strong case for it, whatever I "needed" it for could be achieved by other means, and adding APIs isn't cheap. So, it didn't seem like you actually thought it was even remotely "nice to have".

But maybe I interpreted it wrong too :).

Of course nothing is blocking me on this, and I'd love to see it incorporated at whichever time is most appropriate.

Thanks!

[tarekgh]

First time I get this mpression from you

Sorry to have you got this impression. this is definitely was not my intention. I was just focusing on the importance of the API to have it now as we are late in the game now. anyway, thanks for your feedback.

[kzu]

Yeah, no worries. I didn't mean to imply this was necessary for .NET5 either 👍