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

[Erik Gahlin]

On Thu, 13 Oct 2022 06:07:49 GMT, Chihiro Ito <cito 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
>
> Chihiro Ito has updated the pull request incrementally with one additional commit since the last revision:
> 
>   reverting file headers

What worries me a bit is the testing. It is thorough, but perhaps too thorough? 

It might fail due to other reasons than the added logic, or it may become a maintenance burden. For example, the flush-interval option is planned to be removed. Starting 40 JVMs to verify an improved error message seems excessive. 

I would put the tests in one file, TestConflictingOptions and use options that are harmless (disk and name), but include "settings" as it should be repeatable, i.e 

    void testJCmdConflict() {
      var output= JcmdHelper.jcmd("JFR.start name=hello name=greetings");
      output.shouldContain("name can only be specified once")
    }

    void testStartFlightRecordingConflict() {
     var output = ProcessTools.executeTestJava("-XX:StartFlightRecording:disk=true,disk=false,name=cat,name=dog");
     output.shouldContain("disk and name can only be specified once.")
    }

    void testSettings() {
     var output = ProcessTools.executeTestJava("-XX:StartFlightRecording:settings=default,settings=profile");
     output.shouldNotContain("setting can only be specified once")
    }

This should give 100% line coverage and make sure it works from both command line and jcmd.

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

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