Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

evgenyfedorov2
Copy link
Contributor

@evgenyfedorov2 evgenyfedorov2 commented Oct 6, 2024

Fixes #5472

Microsoft Reviewers: Open in CodeFlow

@evgenyfedorov2 evgenyfedorov2 self-assigned this Oct 6, 2024
@evgenyfedorov2 evgenyfedorov2 changed the title Resource Monitoring metrics on Window - remove multiplication by 100 Resource Monitoring metrics on Windows - remove multiplication by 100 Oct 6, 2024
@evgenyfedorov2 evgenyfedorov2 marked this pull request as ready for review October 6, 2024 20:29
@RussKie
Copy link
Member

RussKie commented Oct 7, 2024

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

@RussKie RussKie added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Oct 7, 2024
@evgenyfedorov2
Copy link
Contributor Author

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.

@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Oct 8, 2024
@@ -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);
Copy link

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?

Copy link
Member

@RussKie RussKie left a 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.

@dotnet-policy-service dotnet-policy-service bot added the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Oct 10, 2024
@joperezr
Copy link
Member

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.

@RussKie
Copy link
Member

RussKie commented Oct 28, 2024

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

@joperezr
Copy link
Member

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.

@evgenyfedorov2
Copy link
Contributor Author

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?

@dotnet-policy-service dotnet-policy-service bot removed the waiting-author-feedback 📭 The author of this issue needs to respond in order for us to continue investigating this issue. label Oct 29, 2024
@joperezr
Copy link
Member

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.

@evgenyfedorov2
Copy link
Contributor Author

evgenyfedorov2 commented Oct 30, 2024

  • 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource Monitoring metrics for Windows report utilization in the [0,100] range
4 participants