JMC-5639: Can't resolve event type names without Recording Settings event

[Marcus Hirt]

Took a closer look at this just now, and realized that there is one additional

change required:

 

http://cr.openjdk.java.net/~hirt/JMC-5663/webrev.03/

 

Kind regards,

Marcus

 

On 2018-09-19, 06:29, "jmc-dev on behalf of Marcus Hirt" <jmc-dev-bounces at openjdk.java.net on behalf of marcus.hirt at oracle.com> wrote:

 

    Decided to change a variable name and remove unnecessary newline:

    http://cr.openjdk.java.net/~hirt/JMC-5663/webrev.02/

    

    Kind regards,

    Marcus

    

    On 2018-09-19, 06:23, "jmc-dev on behalf of Marcus Hirt" <jmc-dev-bounces at openjdk.java.net on behalf of marcus.hirt at oracle.com> wrote:

    

        Added a webrev here:

        http://cr.openjdk.java.net/~hirt/JMC-5663/webrev.01/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java.cdiff.html

        

        Kind regards,

        Marcus

        

        On 2018-09-19, 06:09, "jmc-dev on behalf of Marcus Hirt" <jmc-dev-bounces at openjdk.java.net on behalf of marcus.hirt at oracle.com> wrote:

        

            Hi Ken,

            

            I would get the distinct event types through an aggregator rather than 

            iterating the IItemIterables, like so:

            

               private static String getEventTypeNames(IItemCollection items) {

                               Set<String> names = items.getAggregate(Aggregators.distinct("", TYPE_NAME_ACCESSOR_FACTORY)); //$NON-NLS-1$

                               List<String> quotedNames = new ArrayList<>();

                               if (names == null || names.isEmpty()) {

                                               Set<IType<?>> types = items.getAggregate(Aggregators.distinct("", JfrAttributes.EVENT_TYPE)); //$NON-NLS-1$

                                               for (IType<?> type : types) {

                                                               quotedNames.add("'" + type.getIdentifier() + "'"); //$NON-NLS-1$ //$NON-NLS-2$

                                               }

                               } else {

                                               for (String name : names) {

                                                               quotedNames.add("'" + name + "'"); //$NON-NLS-1$ //$NON-NLS-2$

                                               }

                               }

                               Collections.sort(quotedNames);

                               return StringToolkit.join(quotedNames, ", "); //$NON-NLS-1$

               }

            

            Note that both of these implementations, whilst probably good enough in 

            practice, are not strictly correct. If there are only some types lacking 

            recording settings, they will not be listed. This means the customer may need 

            to make several attempts before finding them all. A fully correct 

            implementation would compare the set of type ids available in the collection, 

            to the set of type ids represented in the recording settings, and only add the 

            type ids for the type ids missing in the recording settings.

            

            A smaller and simpler correct fix, given that the TYPE_NAME_ACCESSOR_FACTORY 

            is only used by the getEventTypeNames method, would probably be to edit the 

            TYPE_NAME_ACCESSOR_FACTORY to something like this:

            

               /*

                * Returns the type name, as available in the recording settings, or simply the type id, if not available.

                */

               private static final IAccessorFactory<String> TYPE_NAME_ACCESSOR_FACTORY = new IAccessorFactory<String>() {

                               @Override

                               public <T> IMemberAccessor<String, T> getAccessor(final IType<T> type) {

                                               final IMemberAccessor<LabeledIdentifier, T> ta = JdkAttributes.REC_SETTING_FOR.getAccessor(type);

                                               return new IMemberAccessor<String, T>() {

                                                               @Override

                                                               public String getMember(T inObject) {

                                                                               LabeledIdentifier eventType = ta.getMember(inObject);

                                                                               return eventType == null ?  type.getIdentifier() : eventType.getName();

                                                               }

            

                                               };

                               }

               };

            

            Hope this helps!

            

            Kind regards,

            Marcus

            

            On 2018-09-18, 14:08, "jmc-dev on behalf of Ken Dobson" <jmc-dev-bounces at openjdk.java.net on behalf of kdobson at redhat.com> wrote:

            

                Hi all,

                

                This is a pretty simple fix for

                https://bugs.openjdk.java.net/projects/JMC/issues/JMC-5663 if someone could

                please review it that would be great.

                

                diff -r 49f6575169ce

                core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java

                ---

                a/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java

                Tue Jul 31 20:02:30 2018 +0200

                +++

                b/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java

                Wed Sep 12 16:11:17 2018 -0400

                @@ -952,12 +952,16 @@

                

                     private static String getEventTypeNames(IItemCollection items) {

                         Set<String> names = items.getAggregate(Aggregators.distinct("",

                TYPE_NAME_ACCESSOR_FACTORY)); //$NON-NLS-1$

                -        if (names == null) {

                -            return null;

                +        List<String> quotedNames = new ArrayList<>();

                +        if (names == null || names.isEmpty()) {

                +            for(IItemIterable iter : items){

                +                quotedNames.add("'" + iter.getType().getIdentifier() +

                "'");

                +            }

                         }

                -        List<String> quotedNames = new ArrayList<>();

                -        for (String name : names) {

                -            quotedNames.add("'" + name + "'"); //$NON-NLS-1$ //$NON-NLS-2$

                +        else {

                +            for (String name : names) {

                +                quotedNames.add("'" + name + "'"); //$NON-NLS-1$

                //$NON-NLS-2$

                +            }

                         }

                         Collections.sort(quotedNames);

                         return StringToolkit.join(quotedNames, ", "); //$NON-NLS-1$

                

                Thanks,

                

                Ken