extensions icon indicating copy to clipboard operation
extensions copied to clipboard

Resource Monitoring metrics on Windows - remove multiplication by 100

Open evgenyfedorov2 opened this issue 1 year ago • 4 comments

Fixes #5472

Microsoft Reviewers: Open in CodeFlow

evgenyfedorov2 avatar Oct 06 '24 19:10 evgenyfedorov2

What's the plan to notify the existing consumers? I imagine dashboards will be affected...

RussKie avatar Oct 07 '24 23:10 RussKie

What's the plan to notify the existing consumers? I imagine dashboards will be affected...

Yes, they will. The only notification mechanism I see is Release Notes.

evgenyfedorov2 avatar Oct 08 '24 09:10 evgenyfedorov2

What's the plan to notify the existing consumers? I imagine dashboards will be affected...

Yes, they will. The only notification mechanism I see is Release Notes.

Yeah so I don't feel comfortable doing this, particularly as we know it is a package that is used internally and we only support the latest version, so we can't make these type of breaking changes (or at least can't take it lightly). Is there a way to do this in a non-breaking way? For instance, we could use quirks where the behavior is preserved, and people that want the new behavior can set an appconfig switch to choose the other behavior. Then we can decide when to switch the defaults.

joperezr avatar Oct 11 '24 18:10 joperezr

@evgenyfedorov2 @joperezr what was the decision on this change?

RussKie avatar Oct 28 '24 00:10 RussKie

We talked about it in the last tactical sync and agreed we couldn't take the change as-is. We suggesting to potentially quirk this and then next major we could switch from opt-in to opt-out. I believe @evgenyfedorov2 was going to work on that.

joperezr avatar Oct 28 '24 18:10 joperezr

We talked about it in the last tactical sync and agreed we couldn't take the change as-is. We suggesting to potentially quirk this and then next major we could switch from opt-in to opt-out. I believe @evgenyfedorov2 was going to work on that.

I was not able to join the meeting, just watched the recording, but I did not hear my argument being addressed. The argument is this: With the quirk, we will eventually make the breaking change some time later when we perhaps have much larger user base. E.g.:

  • if we make this change today, ~0 external customers will be affected (because the Windows metrics part is a brand new feature, only about 2-3 months old) and up to 100 internal ones.
  • if we do this change later in 1-2 years (whenever the next major release is), 100-1000s external and 100+ internal customers will be affected. I don't believe many will opt-in to the right behavior, everyone will just use the default and incorrect behavior.

If you still prefer to support the incorrect behavior, one alternative is to introduce the quirk as proposed, but make the right (new) behavior default, so new customers would onboard to the right behavior by default, while the old customers will get a chance to opt-in to the incorrect behavior if they really want to. What do you think?

evgenyfedorov2 avatar Oct 29 '24 09:10 evgenyfedorov2

There are two things to consider here:

  • Whether or not the new behavior is the correct one that our customers will prefer, as opposed to the previous one. If we aren't sure yet, then having the new behavior be opt-in helps since it doesn't break existing customers, and it allows you to have a controlled amount of customers explicitly try out the new behavior and listen to feedback.
  • Whether or not we can give existing customers the heads up of the change. If we decide this to be opt-out, we should be sure to communicate to all existing customers about the change, as this would break all existing dashboards. If we don't think we can confidently raise awareness of this, I'd prefer this not to be an opt-out behavior.

Ultimately, there is no good answer here that allows you to change behavior, and at the same time provide great compat for existing customers via quirking. Another option that would allow you to do both of those things, would be to introduce net-new metrics, so customers dependent on existing metrics can continue to do so, and we can introduce new ones that provide the new values.

joperezr avatar Oct 29 '24 18:10 joperezr

  • If we aren't sure yet

We are 100% sure, it is a bug (reported to me by one of internal services, actually). This is why this whole thing is confusing to me, generally, bugs are just fixed straight away, regardless whether customers relied on the buggy behavior or not.

evgenyfedorov2 avatar Oct 30 '24 07:10 evgenyfedorov2

Hi, any prorgess or update on this?

makazeu avatar Dec 04 '24 08:12 makazeu

Hi, any prorgess or update on this?

I have different priorities but will try to get back onto it in December/January

evgenyfedorov2 avatar Dec 04 '24 09:12 evgenyfedorov2

I'm blocking this until this is discussed at the Tactical Sync.

As discussed, added a new property UseZeroToOneRangeForMetrics, so that the new (and correct) functionality is now opt-in

evgenyfedorov2 avatar Jan 02 '25 14:01 evgenyfedorov2

:bangbang: Found issues :bangbang:

Project Coverage Type Expected Actual
Microsoft.Extensions.Caching.Hybrid Line 86 84.76 :small_red_triangle_down:

:tada: Good job! The coverage increased :tada: Update MinCodeCoverage in the project files.

Project Expected Actual
Microsoft.Extensions.AI 88 89

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=905312&view=codecoverage-tab

dotnet-comment-bot avatar Jan 02 '25 14:01 dotnet-comment-bot

I'm blocking this until this is discussed at the Tactical Sync.

As discussed, added a new property UseZeroToOneRangeForMetrics, so that the new (and correct) functionality is now opt-in

Should we set UseZeroToOneRangeForMetrics as a required option to ensure that people are notified of this breaking change?

haipz avatar Jan 14 '25 08:01 haipz

I'm blocking this until this is discussed at the Tactical Sync.

As discussed, added a new property UseZeroToOneRangeForMetrics, so that the new (and correct) functionality is now opt-in

Should we set UseZeroToOneRangeForMetrics as a required option to ensure that people are notified of this breaking change?

Eventually we will want to set UseZeroToOneRangeForMetrics = true by default, and later on remove that property completely. The required modifier might complicate things

evgenyfedorov2 avatar Jan 20 '25 16:01 evgenyfedorov2

@evgenyfedorov2 please link here the PR updating the docs. Thank you.

RussKie avatar Feb 02 '25 23:02 RussKie