RFR: 8223066: "jfr metadata" output the @Name annotation twice.

[Chihiro Ito]

Hi Erik,

Thank you for sharing the background.

In the former case, I feel that PrettyWriter strongly depends on the 
name and annotation information of Type.
So I think the latter is better.

Regard
Chihiro

On 2019/05/02 4:14, Erik Gahlin wrote:
> Hi Chihiro,
>
> Thanks for reporting the issue, @Name should not be printed twice.
>
> Let me give you some background information. I think there are two 
> issues at hand:
>
> 1) The Name annotation should probably not be persisted. This can be 
> achieved by removing the MetadataDefinition from the class. I did try 
> that, but I noticed several tests failing, in particular TestName 
> which explicitly checks that @Name is persisted. The purpose @Name is 
> to override the default name, which can then be retrieved from 
> ValueDescriptor::getName, EventType::getName and 
> SettingDescriptor::getName. The annotation is not needed on the 
> consumer side as the name is already persisted elsewhere.
>
> I remember discussing this several times during the design, but for 
> some reason we came to the conclusion to persist it, probably to not 
> confuse users if it didn't behave like @Label and @Description. 
> Later,  Metadatadefinition was removed from @Threshold, @Periodic, 
> @Enabled and @StackTrace. At that time, it should probably have been 
> removed from @Name as well. Maybe we should go ahead and remove it 
> now, but I will have to think about the implications first.
>
> 2) The reason PrettyWriter::printType adds @Name is because there is 
> no good way to include package/namespace in the printout. Events that 
> lack a namespace should not get the annotation, that's why there is a 
> check for "." It's not because the values are primitives.
>
> I can see two ways to proceed. Either add a check in printType so 
> @Name is only printed if it doesn't already exists, i.e.
>
> if (t.getAnnotation(Name.class) == null) {
>   if (index != -1) {
>     println("@Name(\"" + typeName + "\")");
>   }
> }
>
>  or remove @MetadataDefinition from the Name class and make 
> adjustments to tests etc. If you chose the latter, I like a week to 
> think it over, investigate the implications and discuss it with other 
> people, before I can say if it is the right approach or not.
>
> Thanks
> Erik
>
> Hi,
>>
>> I fixed an issue that the @Name annotation was printed twice as 
>> followings in "jfr metadata" output.
>>
>> @Name("jdk.ActiveSetting")
>> @Name("jdk.ActiveSetting")
>> @Label("Recording Setting")
>> @Category("Flight Recorder")
>> class ActiveSetting extends jdk.jfr.Event {
>>
>>
>> Could you reveiew this please?
>>
>> webrev:
>> http://cr.openjdk.java.net/~cito/JDK-8223066/webrev.00/ 
>> <http://cr.openjdk.java.net/%7Ecito/JDK-8223066/webrev.00/>
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8223066
>>
>> I have passed all the tests in test-tier1.
>>
>> Best regards,
>> Chihiro
>
>