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

[Marcus Hirt]

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