search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.06k stars 4.69k forks source link

Mark Activity.Tags as obsolete and/or create an analyzer #49383

Open msallin opened 3 years ago

msallin commented 3 years ago

The Activity.Tags property only return tags which are KeyValuePair<string, string> and shouldn't be used anymore. Having both without any hints about this is confusing for the customer and might lead to mistakes.

Don't use anymore: https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L271-L280

Use: https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L282-L289

For details see discussion with @tarekgh https://github.com/dotnet/runtime/pull/48722#discussion_r589087042

ghost commented 3 years ago

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

Issue Details
The `Activity.Tags` property only return tags which are `KeyValuePair` and shouldn't be used anymore. Having both without any hints about this is confusing for the customer and might lead to mistakes. Don't use anymore: https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L271-L280 Use: https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L282-L289 For details see discussion with @tarekgh https://github.com/dotnet/runtime/pull/48722#discussion_r589087042
Author: msallin
Assignees: -
Labels: `area-System.Diagnostics.Tracing`, `untriaged`
Milestone: -
ghost commented 3 years ago

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

Issue Details
The `Activity.Tags` property only return tags which are `KeyValuePair` and shouldn't be used anymore. Having both without any hints about this is confusing for the customer and might lead to mistakes. Don't use anymore: https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L271-L280 Use: https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Activity.cs#L282-L289 For details see discussion with @tarekgh https://github.com/dotnet/runtime/pull/48722#discussion_r589087042
Author: msallin
Assignees: -
Labels: `area-System.Diagnostics.Activity`, `area-System.Diagnostics.Tracing`, `code-analyzer`
Milestone: -
stephentoub commented 1 year ago

@tarekgh, is this really a code analyzer, or are you just considering obsoleting the property if it really shouldn't be used anymore?

tarekgh commented 1 year ago

It is more considering obsoletion. The analyzer will be needed only if we don't obsolete it.

CC @noahfalk

menees commented 1 year ago

Yes, please mark Tags as obsolete in .NET 8. I couldn't find anything in the .NET 7 docs about which one to use or prefer. Finally, a web search for "activity tags vs tagobjects" linked to this issue as the 7th link down. That's too much of a treasure hunt to figure out the preferred API. An [Obsolete] attribute on Tags that said to use TagObjects instead would be much better.