Review request JMC-6512: for Competing CPU load rule should take overall load into account

[Florian David]

I updated the patch and tests are passing. I also corrected 2 more messages
(CompareCpuRule_INFO_LIMIT and CompareCpuRule_WARNING_LIMIT).
I have also rebased the patch so that it can be applied to the last
revision (659b7d0350d6).

Link: http://cr.openjdk.java.net/~hirt/fdavid/JMC-6512/webrev.2/

Le lun. 8 juil. 2019 à 11:25, Marcus Hirt <marcus.hirt at datadoghq.com> a
écrit :

> Jira ticket opened here:
> https://bugs.openjdk.java.net/browse/JMC-6523
>
> /M
>
> On Mon, Jul 8, 2019 at 10:59 AM Jie Kang <jkang at redhat.com> wrote:
>
>> On Wed, Jul 3, 2019 at 11:52 AM Florian David <florian.david at gmail.com>
>> wrote:
>> >
>> >
>> >
>> > Le jeu. 27 juin 2019 à 23:38, Jie Kang <jkang at redhat.com> a écrit :
>> >>
>> >> Hi Florian,
>> >>
>> > Hi Jie,
>> >
>> > Thanks for the review. Please find the new one taking into account your
>> feedback here:
>> > http://cr.openjdk.java.net/~hirt/fdavid/JMC-6512/webrev.1/
>> >
>> >>
>> >> Changes look mostly good.
>> >>
>> >> # I have some suggestions and comments below:
>> >>
>> >> * The Messages also contain:
>> >>
>> >> CompareCpuRule_INFO_LIMIT=Competing CPU usage limit
>> >> CompareCpuRule_INFO_LIMIT_LONG=The amount of CPU used by other
>> >> processes needed to trigger an info notice
>> >> CompareCpuRule_WARNING_LIMIT=Competing CPU usage warning limit
>> >> CompareCpuRule_WARNING_LIMIT_LONG=The amount of CPU used by other
>> >> processes needed to trigger a warning
>> >>
>> >> I think this patch should also alter these descriptions to reflect the
>> >> new calculation behind the rule.  To be honest I'm not sure how to
>> >> describe the new meaning succinctly in words, otherwise I'd provide a
>> >> suggestion, sorry;
>> >>
>> > I have updated these messages:
>> >
>> > CompareCpuRule_TEXT_INFO_LONG=The application performance can be
>> affected when the machine is under heavy load and there are other processes
>> that use CPU or other resources on the same computer. To profile
>> representatively or get higher throughput, shut down other resource
>> intensive processes running on the machine.
>> > ...
>> > CompareCpuRule_TEXT_MESSAGE=An average CPU load of {2} was caused by
>> other processes during {0} starting at {1}.
>> > CompareCpuRule_INFO_LIMIT=Competing CPU usage info limit
>> > CompareCpuRule_INFO_LIMIT_LONG=The amount of CPU used by other
>> processes times the overall CPU usage needed to trigger an info notice
>> > CompareCpuRule_WARNING_LIMIT=Competing CPU usage warning limit
>> >
>> > I tried to keep these messages simple and not overwhelming the user
>> with lot of details.
>> > However, if you think it should be even more detailed, I can be more
>> precise.
>>
>> Hey Florian,
>>
>> These messages are fine with me. Unfortunately some of the tests now fail:
>>
>> [ERROR] Failures:
>> [ERROR]   TestRulesWithJfr.verifyAllResults:132->verifyRuleResults:159
>>
>> I am guessing the JfrRuleBaseline.xml needs to have some more updates
>> for the new changes.
>>
>> I think the rest of the patch looks fine!
>>
>> >
>> >>
>> >> # The following comments could be addressed in different Jira issues,
>> if at all:
>> >>
>> >> * The SpanToolkit is used twice on the items now, once for the max and
>> >> once for the ratio. Could the underlying iterations over the items be
>> >> combined? I understand they calculate different spans, one of which is
>> >> used in the message while the other is used for the score. Could they
>> >> be aligned?
>> >>
>> >> * It's curious that the SpanToolkit is given the warning limit whereas
>> >> the long message is generated if the score is above the info limit. As
>> >> well, the warning limit is used to determine if spans get combined
>> >> while the end result used for the score process is the longest span,
>> >> regardless of it's value's relation to the warning limit. Should it be
>> >> the longest span that's above the warning limit, if possible,
>> >> otherwise the longest span? I guess it depends on if the initial spans
>> >> vary in length or not. I'm not sure how to describe it but something
>> >> feels strange here.
>> >>
>> > I agree, these comments are relevant. However I'm not yet a contributor
>> > thus I don't have rights to create Jira tickets :( How can we address
>> that ?
>>
>> I think you can ask Marcus to create a Jira ticket for you.
>>
>>
>> Regards,
>>
>>
>>
>> >
>> > For the second point, I also believe there is something fishy in this
>> code
>> > about using the warning limit instead of the info limit is incorrect.
>> I'm not
>> > mastering spans already but passing warning limit to getMaxSpanLimit
>> > should exclude span below this limit, thus only triggering the rule if
>> score
>> > is above warning limit only and never of it's between info and warning
>> limits.
>> > Something is wrong in this workflow.
>> >
>> >> # The following is just a question for the intended design:
>> >>
>> >> * After an initial read of these changes, I would expect that if I set
>> >> a warning limit of say 0.4, then a warning would be triggered if there
>> >> is a longest span where the CPU usage of other processes was 40% and
>> >> the total CPU usage was 100% I.e. the range is like [1]? Is that
>> >> right?
>> >>
>> >> [1]
>> >> x*y<LIMIT, 0<=x<=1, 0<=y<=1, x<y
>> >> x: CPU Usage of Other Processes over the longest span of time
>> >> y: CPU Usage of All Processes over longest span of time
>> >>
>> > Yes that's exactly the computation being done. The core idea is that
>> when
>> > machine CPU is low, it's meaningless to trigger the rule since there is
>> still enough
>> > CPU to run other processes. The multiplication by 'y' decreases the
>> score
>> > in this case.
>> >
>> > Regards,
>> > Florian
>> >
>> >>
>> >>
>> >> Regards,
>> >>
>> >>
>> >> On Thu, Jun 27, 2019 at 5:34 AM Florian David <florian.david at gmail.com>
>> wrote:
>> >> >
>> >> > Hi all,
>> >> >
>> >> > Please review this fix for the CPU load rule.
>> >> >
>> >> > JIRA: https://bugs.openjdk.java.net/browse/JMC-6512
>> >> > Webrev: http://cr.openjdk.java.net/~hirt/fdavid/JMC-6512/webrev.0/
>> >> >
>> >> > Regards,
>> >> > Florian
>>
>