RFR: 8051959: Add thread and timestamp options to java.security.debug system property [v3]

[Weijun Wang]

On Thu, 21 Mar 2024 19:39:36 GMT, Sean Coffey <coffeys at openjdk.org> wrote:

>> Proposal to improve the `java.security.debug` output so that options exist to add thread ID, thread name, source of log record and a timestamp information to the output.
>> 
>> examples:
>> format without patch :
>> 
>> 
>> properties: Initial security property: package.definition=sun.misc.,sun.reflect.
>> properties: Initial security property: krb5.kdc.bad.policy=tryLast 
>> keystore: Creating a new keystore in PKCS12 format
>> 
>> 
>> format with thread info included:
>> 
>> 
>> properties[10|main|Security.java:122]: Initial security property: package.definition=sun.misc.,sun.reflect.
>> properties[10|main|Security.java:122]: Initial security property: krb5.kdc.bad.policy=tryLast 
>> keystore[10|main|KeyStoreDelegator.java:216]: Creating a new keystore in PKCS12 format
>> 
>> 
>> format with thread info and timestamp:
>> 
>> 
>> properties[10|main|Security.java:122|2024-03-01 14:59:42.859 UTC]: Initial security property: package.definition=sun.misc.,sun.reflect.
>> properties[10|main|Security.java:122|2024-03-01 14:59:42.859 UTC]: Initial security property: krb5.kdc.bad.policy=tryLast
>> 
>> 
>> It's a similar format to what can be seen when the TLS (javax.net.debug) debug logging option is in use
>> 
>> current proposal is to keep the thread and timestamp information off (make it opt in)
>> 
>> The extra decorator info is controlled by appending option to each component specified in the `"java.security.debug"` option list.
>> 
>> e.g 
>> 
>> `-Djava.security.debug=properties+timestamp+thread` turns on logging for the `properties` component and also decorates the records with timestamp and thread info
>> 
>> -Djava.security.debug=properties+thread+timestamp,keystore would decorate the `properties` component but no decorating performed for the `keystore `component.
>
> Sean Coffey has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision:
> 
>  - merge in krb5 debug changes and implement extra functionality. enhance krb5 test
>  - krb5 debug merge
>  - Merge branch 'master' into 8051959-tracing
>  - Move help section higher. Add code comment
>  - more test coverage
>  - unused variable
>  - use default hex output
>  - static dateTimeFormatInitialized
>  - Merge branch 'master' into 8051959-tracing
>  - Holder class idiom
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/a33f4bde...5628bc22

src/java.base/share/classes/sun/security/util/Debug.java line 191:

> 189:         if (printDateTime && !dateTimeFormatInitialized) {
> 190:             // trigger loading of Locale service impl now to avoid
> 191:             // possible bootstrap recursive class load issue

Does this need to be manually loaded? I thought the `FormatHolder` field will be automatically loaded when it's first accessed and only once.

src/java.base/share/classes/sun/security/util/Debug.java line 206:

> 204: 
> 205:     // parse an option string to determine if extra details
> 206:     // (like thread and timestamp) should be printed

I find this method (and maybe also `configureOptions`) a little confusing. The `option` argument is the option name for `getInstance` but the property value for `of`. You can see that in this method there is completely no shared code between `ofInstance` being true or false.

src/java.base/share/classes/sun/security/util/Debug.java line 257:

> 255:      * }
> 256:      * @param prefix the debug option name
> 257:      * @param property debug setting for this option

Please add some words on the format that `property`can take.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18084#discussion_r1534645297
PR Review Comment: https://git.openjdk.org/jdk/pull/18084#discussion_r1534645506
PR Review Comment: https://git.openjdk.org/jdk/pull/18084#discussion_r1534645944