Unify with logging category matching#90559
Conversation
ghost
added
the
needs-area-label
jkotas
added
area-System.Diagnostics.Metric
and removed
needs-area-label
src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/ListenerSubscription.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Diagnostics/src/Metrics/ListenerSubscription.cs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| if (!instrument.Meter.Name.AsSpan().StartsWith(prefix, StringComparison.OrdinalIgnoreCase) || | ||
| !instrument.Meter.Name.AsSpan().EndsWith(suffix, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
what the behavior will be if have like, meter name ABCDE and have searching for ABC*CDE. This search should fail but with current code it will succeed I guess. Maybe we need to check the length of prefix and suffix against the meter name?
There was a problem hiding this comment.
Since we're going for complete compatibility with logging's matching I'd rather not adjust the behavior for corner cases like this. We can revisit this in 9 for both of them if you want.
There was a problem hiding this comment.
Would it be a good idea to stick with the wrong behavior? This will be compatibility issue if we don't do it from now. I prefer doing it but I'll leave it to you if you think otherwise.
There was a problem hiding this comment.
It does match when it shouldn't. Even weirder, ABCDE*ABCDE matches "ABCDE" since it's just StartsWith and EndsWith checks. I'm going to leave it for now since logging works the same way.
There was a problem hiding this comment.
I agree its weird, but my rationale has remained that if people aren't complaining about it via ILogger then we should remain compatible. If someone did complain about it I assume we'd want to change it uniformly, not change it just for metrics.
could you please add a test for the case I mentioned earlier? meter name Refers to: src/libraries/Microsoft.Extensions.Diagnostics/tests/ListenerSubscriptionTests.cs:258 in 6b0484e. [](commit_id = 6b0484e, deletion_comment = False) |
tarekgh
left a comment
tarekgh
left a comment
There was a problem hiding this comment.
Added a few comments/questions. Otherwise LGTM.
|
/backport to release/8.0 |
|
/backport to release/8.0-rc1 |
|
Started backporting to release/8.0-rc1: https://github.com/dotnet/runtime/actions/runs/5872147388 |
4 participants
Follow up to #90201. I'd implemented the meter name matching to follow logging's documented behaviors, and what we'd thought was plausibly useful. However, logging's category matching has a lot of undocumented behaviors and odd assumptions.
This change makes meter matching work the same as category matching.