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

[Marcus Hirt]

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