RFR - [RFE] JDK-8007141 : Introduce Dynamic MBean exposing the perf counters

[Harsha Wardhana B]

Hi Daniel,


On Tuesday 07 March 2017 08:40 PM, Daniel Fuchs wrote:
> Hi Harsha,
>
> Not a review either (at least not a complete one).
>
> PlatformMBeanProviderImpl:
>
>  281             @Override
>  282             public Set<Class<? extends DynamicMBean>> 
> mbeanInterfaces() {
>  283                 return Collections.emptySet();
>  284             }
>
> For future maintainers, I think it would be good to copy
> the comment at line 250 and insert it before line 283 too:
>  250                     // DynamicMBean cannot be used to find an 
> MBean by ManagementFactory
>
Will do.
> HotSpotPerfCounterMBean:
>
> Exception handling: I have the feeling that this part might
> be a bit over-engineered. If you look at javax.management.StandardMBean
> (which is a canonical implementation of DynamicMBean) and
> then transitively at MBeanSupport and PerInterface where this class
> eventually delegates to, you will see that:
>
> 1. You can throw NPE in setAttribute when attribute == null (no need for
>    all the wrapping)
If attribute == null, isn't it better to throw IllegalArgumentException 
instead of NPE? I'll remove the wrapping.
> 2. You should throw AttributeNotFoundException when attempting to read
>    a write-only attribute or write a read-only attribute
Isn't UnSupportedOperationEx better than AttributeNotFoundException? 
Have we committed to above in the specs?
> 3. You can throw NPE if AttributeList is null
If attribute == null, isn't it better to throw IllegalArgumentException 
instead of NPE? I'll remove the wrapping.
>
>  179             String typeName = "java.lang.String";
>
> That looks like a hack.
If attribute is null, defaulting to String would be convenient as 
getAttribute can return empty string instead of null. Do you think 
otherwise?
>
> What if at some point the value of that attribute becomes non null,
> and its class is *not* java.lang.String?
> Then MBeanAttributeInfo would be lying.
> If that ever happens - then it would be more consistent to coerce
> the value to its declared type (String) before sending it as a
> a result for getAttribute.
Since perf-counters are key-value pairs written into shared memory, it 
is reasonable to expect new counters to be added or type of existing 
counters to be updated while VM is running. I have to refresh the 
counters periodically (maybe once every 5 mins) since I will not be 
notified of any change. If any attributes are added or if existing 
attribute types are changed, then AttributeChangeNotification will be 
sent. Clients will have to refresh MBeanInfo upon receiving this 
notification. So yeah, there is a brief window where MBeanInfo will be 
lying. I don't see easy way to fix it unlesss perfcounters can notify 
MBean of any mutations. Maybe the limitation can be documented.

I will make these changes in next webrev.

> Or better, find a way to figure out what should be the class name
> of the value even if the value is null (isn't there some metadata
> available for these perf counters somewhere?).
>
> PerfCounterMBeanTest:
>
> testGetAttribute should verify that class of the returned value
> corresponds to what was declared in the MBeanAttributeInfo for the
> corresponding attribute.
Sure. Will do.
>
> cheers,
>
> -- daniel
-Harsha
>
> On 26/02/17 16:50, Harsha Wardhana B wrote:
>> Hi All,
>>
>> Please review the below RFE,
>>
>> JDK-8007141 <http://JDK-8007141> : Introduce Dynamic MBean exposing the
>> perf counters.
>>
>> with webrev at,
>>
>> http://cr.openjdk.java.net/~hb/8007141/webrev.00/
>>
>> Appreciate if I can get inputs for below.
>>
>> 1. Location of HotSpotPerfCounterMBean. It is located at
>> src/jdk.management/share/classes/com/sun/management/internal. It can be
>> moved to src/jdk.management/share/classes/com/sun/management if 
>> required.
>>
>> 2. Javadoc comments for HotSpotPerfCounterMBean. Not sure if it is 
>> adequate.
>>
>> 3. Description for each attribute of MBean. I am using getUnits(),
>> getVariability(), and getFlags() for description. I am not sure if that
>> is the right description. Appreciate inputs from someone who knows perf
>> counters well.
>>
>> Thanks
>>
>> Harsha
>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20170308/75a48840/attachment-0001.html>