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

[Erik Gahlin]

Hi Chihiro ,

I haven't had time to discuss alternative 1, if that is what you want to 
proceed with.

If you want alternative 2, I don't think changes to the "low-level" 
implementation (MetadataReader) is the correct way to go about it. It 
will impact other code, not just the 'jfr metadata' command.

Erik

> Hi Erik,
>
> I removed @Name from the annotations.
> Could you review it?
>
> webrev:
> http://cr.openjdk.java.net/~cito/JDK-8223066/webrev.01/ 
> <http://cr.openjdk.java.net/%7Ecito/JDK-8223066/webrev.01/>
>
> JBS:
> https://bugs.openjdk.java.net/browse/JDK-8223066
>
>
> The output look like this.:
>
> @Name("jdk.ActiveSetting")
> @Label("Recording Setting")
> @Category("Flight Recorder")
> class ActiveSetting extends jdk.jfr.Event {
>
>
> Regards,
> Chihiro
>
> On 2019/05/04 2:48, Chihiro Ito wrote:
>> 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
>>>
>>>
>>
>>
>