RFR: 8148188: Enhance the security libraries to record events of interest

[Seán Coffey]

I'd like to re-boot this review. I've been working with Erik off thread 
who's been helping with code and test design.

Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name : 
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the @Relational 
JFR annotation.
* Keep calls for JFR event operations local to call site to optimize jvm 
compilation

webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/

This code is dependent on the JDK-8203629 enhancement being integrated.

Regards,
Sean.

On 10/07/18 13:50, Seán Coffey wrote:
> Erik,
>
> After some trial edits, I'm not so sure if moving the event & logger 
> commit code into the class where it's used works too well after all.
>
> In the code you suggested, there's an assumption that calls such as 
> EventHelper.certificateChain(..) are low cost. While that might be the 
> case here, it's something we can't always assume. It's called once for 
> the JFR operation and once for the Logger operation. That pattern 
> multiplies itself further in other Events. i.e. set up and assign the 
> variables for a JFR event without knowing whether they'll be needed 
> again for the Logger call. We could work around it by setting up some 
> local variables and re-using them in the Logger code but then, we're 
> back to where we were. The current EventHelper design eliminates this 
> problem and also helps to abstract the recording/logging functionality 
> away from the functionality of the class itself.
>
> It also becomes a problem where we record events in multiple different 
> classes (e.g. SecurityPropertyEvent). While we could leave the code in 
> EventHelper for this case, we then have a mixed design pattern.
>
> Are you ok with leaving as is for now? A future wish might be one 
> where JFR would handle Logger via its own framework/API in a future 
> JDK release.
>
> I've removed the variable names using underscore. Also optimized some 
> variable assignments in X509Impl.commitEvent(..)
>
> http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/
>
> regards,
> Sean.
>
> On 09/07/2018 18:01, Seán Coffey wrote:
>> Erik,
>>
>> Thanks for reviewing. Comments inline..
>>
>> On 09/07/18 17:21, Erik Gahlin wrote:
>>> Thanks Sean.
>>>
>>> Some feedback on the code in the security libraries.
>>>
>>> - We should use camel case naming convention for variables (not 
>>> underscore).
>> Sure. I see two offending variable names which I'll fix up.
>>>
>>> - Looking at sun/security/ssl/Finished.java,
>>>
>>> I wonder if it wouldn't be less code and more easy to read, if we 
>>> would commit the event in a local private function and then use the 
>>> EventHelper class for common functionality.
>> I'm fine with your suggested approach. I figured that tucking the 
>> recording/logging logic away in a helper class also had benefits but 
>> I'll re-factor to your suggested style unless I hear objections.
>>
>> regards,
>> Sean.
>>
>