[PATCH] JMC-5499 Add XFlagChanged events to JVM Information or subpage

[Jessye Coleman Shapiro]

Hi,

Here is the webrev for an updated patch:
http://cr.openjdk.java.net/~aptmac/JMC-5499/webrev.02/

On Thu, Jul 4, 2019 at 1:43 PM Alex Macdonald <almacdon at redhat.com> wrote:
>
> On Thu, Jul 4, 2019 at 11:43 AM Jessye Coleman Shapiro <
jescolem at redhat.com> wrote:
>>
>> Here is an updated version of the patch:
>>  http://cr.openjdk.java.net/~aptmac/JMC-5499/webrev.01/
>> <http://cr.openjdk.java.net/~aptmac/JMC-5499/webrev.01/>
>>
>> I modified the uitests in JVMInformationPageTest to act as general checks
>> for table population instead of asserting exact table entries.  String
>> comparisons of dates were failing in some environments and not others.
>
>
> The test file looks good, just a couple nits:
> - there will need to be a license header for the file
> - the second test is missing the "Log" part in the method name to
indicate the "LogTable" like written in the previous test
> - the string value for "Old Value" could be retrieved from Messages
> - now that JMC-5327 is pushed there's a folder for flightrecorder page
uitests, this could be placed in there (alongside FileIOPageTest, etc.)
>

I have added those changed to the patch - thanks

> No issues or failures when running these tests:
https://travis-ci.org/aptmac/jmc-qa/builds/554307170
>
>>
>>
>> Let me know what you think,
>>
>> Jessye
>>
>>
>> On Tue, Jul 2, 2019 at 3:56 PM Jessye Coleman Shapiro <
jescolem at redhat.com>
>> wrote:
>>
>> > Here is an updated patch. It includes:
>> >
>> > - A message that reads "No 'Flag Changed' events found".  I didn't add
>> > an explanation for why no events were found because it is possible
>> > that the table could be empty for two reasons simultaneously- some
>> > XFlagChanged events could be disabled and others could have simply not
>> > occurred. However, I am not sure if this message is informative
>> > enough.  I have placed the message under the title of the table - let
>> > me know if this is a good location for it.
>
>
> I think this is good for now. I do agree with Marcus's statement about
not hiding away UI elements that we're not using. My initial inclination
was that the table itself could be not displayed, but because this table is
a component of sash form then the header title would still be visible (and
the sash weights adjusted to reclaim space). However, that's still a
band-aid solution to the bigger problem of better handling empty states.
There is already an issue in JIRA for this sort of behaviour (JMC-4009), so
I'll take a couple ideas and chime in there.
>
>> > - Two uitests that test if the table is correctly populated for a
>> > flight recording that has multiple XFlagChanged events and if the "No
>> > 'Flag Changed' events found" message appears for a flight recording
>> > where no XFlagChanged events occur.
>> > - Removed empty space/lines
>> >
>> > Let me know what you think,
>> >
>> > Jessye
>> >
>> > On Mon, Jun 24, 2019 at 10:58 AM Ken Dobson <kdobson at redhat.com> wrote:
>> > >
>> > > I agree, a simple string added to the table saying "No X events
found" or
>> > > "X event has been disabled" would more than suffice.
>> > >
>> > > Ken
>> > >
>> > >
>> > > On Mon, Jun 24, 2019 at 7:07 AM Marcus Hirt <
marcus.hirt at datadoghq.com>
>> > > wrote:
>> > >
>> > > > Agree. Perhaps now would be a good time to discuss?
>> > > >
>> > > > I am, in general, hesitant to automatically hide UI elements. It
has
>> > been
>> > > > tried in the past (e.g. Microsoft hiding menu elements not used
much),
>> > with
>> > > > little success. That said, we should at the very least have some
>> > feedback
>> > > > showing people whether the lack of data is due to events being
>> > disabled or
>> > > > events not having occurred. What do you guys think?
>> > > >
>> > > > Kind regards,
>> > > > Marcus
>> > > >
>> > > > On Thu, Jun 20, 2019 at 8:35 PM Ken Dobson <kdobson at redhat.com>
wrote:
>> > > >
>> > > >> There's a ticket here about how we should handle empty tables
>> > > >> https://bugs.openjdk.java.net/browse/JMC-4009
>> > > >> I think it's an issue that might have a larger scope than just
this
>> > bug
>> > > >> but
>> > > >> it's definitely worth addressing.
>> > > >>
>> > > >> Ken Dobson
>> > > >>
>> > > >> On Thu, Jun 20, 2019 at 1:30 PM Alex Macdonald <
almacdon at redhat.com>
>> > > >> wrote:
>> > > >>
>> > > >> > On Thu, Jun 20, 2019 at 9:05 AM Jessye Coleman Shapiro <
>> > > >> > jescolem at redhat.com>
>> > > >> > wrote:
>> > > >> >
>> > > >> > > Hi Alex,
>> > > >> > >
>> > > >> > > I will make those changes and send an updated patch.
>> > > >> > >
>> > > >> >
>> > > >> > Err, whoops. I was reviewing your JMC-5706 patch at the same
time as
>> > > >> this
>> > > >> > one, and must have applied the other patch when I thought it was
>> > this
>> > > >> one.
>> > > >> > The previous review actually applies to your JMC-5706 patch, so
I'll
>> > > >> copy
>> > > >> > the review on that thread.
>> > > >> >
>> > > >> > As for this patch:
>> > > >> >
>> > > >> > My initial reaction was that the new table uses up a good
portion
>> > of the
>> > > >> > page real-estate (which is okay), but I'm wondering if the table
>> > should
>> > > >> > only be displayed if it has values to display. Perhaps someone
else
>> > > >> could
>> > > >> > share their opinions as well.
>> > > >> >
>> > > >> > There are empty spaces/lines at the following locations:
>> > > >> > - defaultPages.xml @ line 597
>> > > >> > - JVMInformationPage.java @ lines 122, 162, 187, 227, 231, 258,
262,
>> > > >> 295,
>> > > >> > 296, 303, 305, 342, 346
>> > > >> >
>> > > >> > The build & uitests pass [0], however because a new component
was
>> > added
>> > > >> to
>> > > >> > the UI I'd like to see a uitest to ensure that everything is
>> > working as
>> > > >> > intended.
>> > > >> >
>> > > >> > [0] https://travis-ci.org/aptmac/jmc-qa/builds/548262559
>> > > >> >
>> > > >> >
>> > > >> > >
>> > > >> > > Thank you!
>> > > >> > >
>> > > >> > > Jessye
>> > > >> > >
>> > > >> > > On Wed, Jun 19, 2019 at 3:45 PM Alex Macdonald <
>> > almacdon at redhat.com>
>> > > >> > > wrote:
>> > > >> > >
>> > > >> > >> Hi Jessye,
>> > > >> > >>
>> > > >> > >> On Fri, Jun 7, 2019 at 2:48 PM Jessye Coleman Shapiro <
>> > > >> > >> jescolem at redhat.com> wrote:
>> > > >> > >>
>> > > >> > >>> Hi,
>> > > >> > >>>
>> > > >> > >>> This patch addresses JMC-5499: Add XFlagChanged events to
JVM
>> > > >> > Information
>> > > >> > >>> or subpage [0].  I have added a JVM Flags Log table to the
JVM
>> > > >> > Internals
>> > > >> > >>> Page that displays XFlagChanged events.
>> > > >> > >>>
>> > > >> > >>> Please see the attached patch and let me know what you
think.
>> > > >> > >>>
>> > > >> > >>
>> > > >> > >> The content of this patch look good to me.
>> > > >> > >>
>> > > >> > >> However, there are test failures:
>> > > >> > >>
>> > > >> > >> >
TestRulesWithJfr.verifyAllResults:132->verifyRuleResults:159
>> > > >> > >>
>> > > >> > >> because the generated JfrRuleBaseline.xml doesn't match the
>> > hardcopy
>> > > >> one
>> > > >> > >> used to verify the test. This will need to be updated.
>> > > >> > >>
>> > > >> > >> There's also some empty spaces that can be cleaned up.
>> > > >> > >>
>> > > >> > >> JavaBlockingRule.java
>> > > >> > >> - empty space @ lines 134, 138, 151
>> > > >> > >> - trailing empty space @ line 142
>> > > >> > >>
>> > > >> > >>
>> > > >> > >>>
>> > > >> > >>> Thank you!
>> > > >> > >>>
>> > > >> > >>> Jessye Coleman-Shapiro
>> > > >> > >>>
>> > > >> > >>>  [0] https://bugs.openjdk.java.net/browse/JMC-5499
>> > > >> > >>
>> > > >> > >>
>> > > >> > >> Cheers,
>> > > >> > >>
>> > > >> > >> Alex
>> > > >> > >>
>> > > >> > >
>> > > >> >
>> > > >>
>> > > >
>> >