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
Add Activity.GetTagItem #39866
Comments
@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? |
CC @noahfalk |
@tarekgh This could be achieved if the new That said, I feel like I am forgetting something from the review why was it not done this way. |
@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. |
@tarekgh I see, then this would really be better exposed as a method on |
@SingleAccretion could you please tell more about your scenario and what you are trying to do? #39866 (comment) |
@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. |
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 |
The scenario is exactly the same as for It's a usability improvement when retrieving items. So rather than having to do the foreach at the call site (something It will have the same O-ness as the existing |
FWIW, my concrete scenario is that when ultimately logging from an |
@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. |
Would you care to explain why there is an asymetric API where you have 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 |
@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. |
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). |
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? |
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). |
for tags you can use |
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 |
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. |
@kzu please let me know if you are ok to have this form the next release and nothing is blocking you for now. |
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! |
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. |
Yeah, no worries. I didn't mean to imply this was necessary for .NET5 either 👍 |
Looks good as proposed. namespace System.Diagnostics
{
public class Activity
{
public object? GetTagItem(string key);
}
} |
Background and Motivation
Both
Activity.Tags
andActivity.Baggage
have a somewhat unusualIEnumerable<KeyValuePair<string, string?>>
type, which makes it cumbersome to access individual values (i.e. can't doactivity.Tags.TryGetValue(key, out var value)
). The latter adds a Activity.GetBaggageItem to compensate, but there isn't an equivalent one forTags
.Proposed API
namespace System.Diagnostics { public class Activity { + public object? GetTagItem(string key)
Usage Examples
Alternative Designs
The obvious alternative would be to use the dictionary-style
bool TryGetTag(string key)
instead.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)
tobool TryGetBaggage(string key)
too, I think.I think this alternative proposal is much more idiomatic than the existing
GetBaggageItem
and the proposedGetTagItem
.Risks
None I can think of.
The text was updated successfully, but these errors were encountered: