Add metrics configuration APIs #90201

Tratcher · 2023-08-08T23:41:09Z

Contributes to #85684.

This adds a set of new APIs used to configure metrics and which listeners they should be paired with. The API closely follows the design from logging.

One new package is added, Microsoft.Extensions.Diagnostics.Abstractions.

The components in the Microsoft.Extensions.Diagnostics.Configuration namespace will be implemented in a later PR.

The goal is to have this merged by Thursday afternoon.

…nsole Listener

src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/DebugConsoleMetricListener.cs

tarekgh · 2023-08-10T17:36:52Z

src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/ListenerSubscription.cs

+                    return;
+                }
+
+                if (_instruments.TryGetValue(instrument, out var _))


(_instruments.TryGetValue(instrument, out var _))

Should we ensure the dictionary is created with reference equality comparer? In the future there may be added equality support to the instrument which possibly can make two instruments with the same name/descriptions be equal.

That's not available on all of our TFMs.

Can polyfill this I guess the same way we do like

runtime/src/libraries/Microsoft.Extensions.Options.DataAnnotations/src/Microsoft.Extensions.Options.DataAnnotations.csproj

Line 15 in 79b9707

<Compile Include="$(CoreLibSharedDir)System\Collections\Generic\ReferenceEqualityComparer.cs" />

src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/ListenerSubscription.cs

tarekgh · 2023-08-10T18:12:49Z

src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/ListenerSubscription.cs

+
+            var ruleMeterName = rule.MeterName.AsSpan();
+            // Rule "System.Net.*" matches meter "System.Net" and "System.Net.Http"
+            if (ruleMeterName.EndsWith(".*".AsSpan(), StringComparison.Ordinal))


if (ruleMeterName.EndsWith(".*".AsSpan(), StringComparison.Ordinal))

what happen if someone specify the rule .* only? would it be ok to have this equal to the rule with empty string or * only? I think this may be confusing.

Being discussed over at #90201 (comment)

src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/ListenerSubscription.cs

tarekgh · 2023-08-10T19:01:51Z

src/libraries/Microsoft.Extensions.Diagnostics.Abstractions/src/Metrics/InstrumentRule.cs

+        /// </summary>
+        public MeterScope Scopes { get; } = scopes == MeterScope.None
+            ? throw new ArgumentOutOfRangeException(nameof(scopes), scopes, "The MeterScope must be Global, Local, or both.")
+            : scopes;


scopes;

should we validate this if someone added more unsupported values? or we don't care?

I'm mostly worried about the common case where someone passes in None by mistake. If they want to deliberately pass in other values I'm not going to question them too closely 😁.

I don't have a strong opinion here but would prefer to communicate the failure sooner instead of having the user see unexpected behavior later and it will be hard to investigate.

.../Microsoft.Extensions.Diagnostics.Abstractions/src/Metrics/MetricsBuilderExtensions.Rules.cs

src/libraries/Microsoft.Extensions.Hosting/ref/Microsoft.Extensions.Hosting.cs

...es/Microsoft.Extensions.Diagnostics.Abstractions/tests/MetricsBuilderExtensionsRulesTests.cs

carlossanlop · 2023-08-10T22:57:47Z

CC @gewarren @carlosalberto

Poor @carlosalberto. He always gets tagged by @tarekgh instead of me. 😄

tarekgh · 2023-08-10T23:18:38Z

Poor @carlosalberto. He always gets tagged by @tarekgh Tarek Mahmoud Sayed FTE instead of me. 😄

Sorry @carlosalberto, I promise will be more careful next time.

noahfalk

I'm marking approved so we can get this into RC1. The vast majority looks good but I still disagree with the inconsistent behavior for string matching between metrics and logging. Chris is out tomorrow so any changes there will need to happen in RC2.

tarekgh · 2023-08-11T16:05:13Z

@noahfalk @Tratcher we still have Monday if we need to do anything more before RC1 snap.

xakep139 · 2023-08-17T08:56:51Z

...Extensions.Diagnostics.Abstractions/src/Microsoft.Extensions.Diagnostics.Abstractions.csproj

+    <!-- Disabling baseline validation since this is a brand new package.
+       Once this package has shipped a stable version, the following line
+       should be removed in order to re-enable validation. -->
+    <DisablePackageBaselineValidation>true</DisablePackageBaselineValidation>


Should an issue be created to track that?

We scan all libraries for this and remove it. @ViktorHofer may advise if we need to track individual libraries.

No tracking issue needed. We do this every year around December when the previous .NET version has released. We have this written down in a OneNote.

Tratcher added 30 commits July 25, 2023 10:12

Packaging

919033d

Bulk add types

775c9dd

Configuration packaging, API ref

a537d9e

Hosting API dump, dependencies

7453b2a

AddConsole

784becb

Hosting wireup

6103d82

Rules

f599cb9

Wire up the factory

97aee1f

Revert non-CLS Compliance

646fe7c

Feedback, scopes

7b90ec2

Split DefaultMeterFactory

80dfa86

Remove hosting CLS compliance

4d789be

Split out listener subscriptions.

3bf5e56

Remove filter, add enable, replace TListener with string, internal Co…

74da7e1

…nsole Listener

IMetric_s_SubscriptionManager

0c11591

Better matching

2109487

sealed

a7e1884

Rule heirarchy, tests

f0f578b

Test cases

74bc673

Listener name constant

743d27f

Add/ClearListener

c2a249d

More console output

8275820

Revert DefaultMeterFactory changes

3747367

Cleanup

84eebd6

Notes

5205861

Console updates, SetSoruce

dd8c364

Update InstrumentPublished

fc3553e

Move MetricsBuilderEnableExtensions back

5e042a7

API refactor

2852d14

GetMeasurementHandlers();

98f3eff