Skip to content

Address Metrics APIs issues#86740

Merged
tarekgh merged 6 commits into
dotnet:mainfrom
tarekgh:MetricsAPIsFollowUpDelta
May 26, 2023
Merged

Address Metrics APIs issues#86740
tarekgh merged 6 commits into
dotnet:mainfrom
tarekgh:MetricsAPIsFollowUpDelta

Conversation

@tarekgh
Copy link
Copy Markdown
Member

@tarekgh tarekgh commented May 25, 2023

The changes here addressing the following issues:

@ghost ghost added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels May 25, 2023
@ghost
Copy link
Copy Markdown

ghost commented May 25, 2023

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost assigned tarekgh May 25, 2023
@tarekgh tarekgh requested a review from noahfalk May 25, 2023 01:32
@tarekgh tarekgh added area-System.Diagnostics.Metric and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels May 25, 2023
@tarekgh tarekgh added this to the 8.0.0 milestone May 25, 2023
internal static class DiagnosticsHelper
{
internal static bool CompareTags(IEnumerable<KeyValuePair<string, object?>>? tags1, IEnumerable<KeyValuePair<string, object?>>? tags2)
// This is similar to System.Linq ToArray. We are not using System.Linq here to avoid the dependency.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the dependency problematic?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The auto-instrumentation can force loading higher version of System.Diagnostics.DiagnosticSource than the version of the one that app built with. Increasing the dependency of System.Diagnostics.DiagnosticSource can increase the risk of breaking the auto-instrumentation. Here is some related issue talking about similar thing #42244

Comment thread src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs Outdated
Comment thread src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs Outdated
Comment thread src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs Outdated
@tarekgh
Copy link
Copy Markdown
Member Author

tarekgh commented May 25, 2023

#86567
#77514

Copy link
Copy Markdown
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looked good. I think I spotted two bugs and a few other suggestions inline.

Comment thread src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs Outdated
Comment thread src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs Outdated
Comment thread src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs Outdated
Comment thread src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs Outdated
Comment thread src/libraries/Common/src/System/Diagnostics/DiagnosticsHelper.cs Outdated
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants