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

[Jessye Coleman Shapiro]

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.

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