RFR: 8288783: JFR: Error messages are confusing when options conflict in -XX:StartFlightRecording

[Chihiro Ito]

On Fri, 7 Oct 2022 05:26:44 GMT, Erik Gahlin <egahlin at openjdk.org> wrote:

>> Could I have a review of PR that fixes incorrect error messages regarding of recording.
>> 
>> The current message is very confusing. If a user types jcmd <pid> JFR.start name=abc name=def or java --XX:StartFlightRecording=name=abc,name=def,  the error message says "Duplicates in diagnostic command arguments", but doesn't know what options are duplicated.
>> Furthermore, if a user specifies two or three duplicate parameters, the same logs will be output.
>> 
>> It should say follows,
>> Option name can only be specified once with starting flight recording
>> Options name and disk can only be specified once with starting flight recording
>> Options name, disk and maxage can only be specified once with starting flight recording
>> This problem affects --XX:StartFlightRecording, jcmd JFR.start, JFR.stop, JFR.dump, JFR.check, and MBean.
>> 
>> For other than start, the output is as follows, (example of dump)
>> Option name can only be specified once with dumping flight recording
>> 
>> Testing: jdk/jdk/jfr
>> 
>> Thanks
>> Chihiro
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/dcmd/ArgumentParser.java line 77:
> 
>> 75:     }
>> 76: 
>> 77:     protected void checkConflict() {
> 
> I think this part can be simplified, the variable names are long and there are too many branches/states to track.
> 
> How about this?
> 
>     private void checkConflicted() {
>         if (conflictedOptions.isEmpty()) {
>             return;
>         }
> 
>         StringBuilder sb = new StringBuilder();
>         sb.append("Option");
>         if (conflictedOptions.size() > 1) {
>             sb.append("s ");
>             StringJoiner sj = new StringJoiner(", ");
>             while (conflictedOptions.size() > 1) {
>                 sj.add(conflictedOptions.remove(0));
>             }
>             sb.append(sj);
>             sb.append(" and");
>         }
>         sb.append(" ");
>         sb.append(conflictedOptions.remove(0));
>         sb.append(" can only be specified once.");
>         throw new IllegalArgumentException(sb.toString());
>     }
> 
> It requires an ArrayList, but you could do:
> 
>     if (!conflictedOptions.contains(key)) {
>         conflictedOptions.add(key);
>     }
>           
> I don't think it's worth the complication with the JfrCommand enum to display the correct command being used in an unusual error message.

Thank you for your prompt reviewing.

Your idea is good for me. I will change the code in that regard as soon as I can.

I agree with the complication you mentiond. We posted about this in JBS with phrases like "with -XX:StartFlightRecording" at the end of the log message, and also mentioned jcmd. Howeber, at the existing implementation in logging the message, there is no way to identify a command such as "JFR.start". So I implemented it this way to identify the command as a last resort. 

As you say, I also think it no need to have a log message that identifies each command with the complication. Therefore, I will eliminate this complication.

-------------

PR: https://git.openjdk.org/jdk/pull/9302