-
Notifications
You must be signed in to change notification settings - Fork 751
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resource Monitoring metrics on Windows - remove multiplication by 100 #5473
base: main
Are you sure you want to change the base?
Resource Monitoring metrics on Windows - remove multiplication by 100 #5473
Conversation
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. |
@@ -195,7 +195,7 @@ private double MemoryPercentage(Func<ulong> getMemoryUsage) | |||
{ | |||
if (now >= _refreshAfterMemory) | |||
{ | |||
_memoryPercentage = Math.Min(Hundred, memoryUsage / _memoryLimit * Hundred); // Don't change calculation order, otherwise we loose some precision | |||
_memoryPercentage = Math.Min(Hundred, memoryUsage / _memoryLimit); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that the first parameter Hundred
(defined above as double 100.0d
), was used to 'cap' utilization at 100.0 in the previous version, wouldn't it make sense to 'cap' it at 1.0 going forward?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm blocking this until this is discussed at the Tactical Sync.
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. |
@evgenyfedorov2 @joperezr what was the decision on this change? |
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:
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? |
There are two things to consider here:
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. |
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. |
Fixes #5472
Microsoft Reviewers: Open in CodeFlow