RFR: 8204681: Option to include timestamp in hprof filename [v2]

[Sonia Zaldana Calles]

On Tue, 10 Sep 2024 15:42:57 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> Hi all, 
>> 
>> This PR addresses [8204681](https://bugs.openjdk.org/browse/JDK-8204681) enabling support for timestamp expansion in filenames specified in `-XX:HeapDumpPath` using `%t`. 
>> 
>> As mentioned in this comments for this issue, this is somewhat related to [8334492](https://bugs.openjdk.org/browse/JDK-8334492) where we enabled support for `%p` for filenames specified in jcmd. 
>> 
>> With this patch, I propose: 
>> - Expanding the utility function `Arguments::copy_expand_pid` to `Arguments::copy_expand_arguments` to deal with `%p` expansions for pid and `%t` expansions for timestamps. 
>> - Leveraging the above utility function to enable argument expansion for both heap dump filenames and jcmd output commands. 
>> - Though the linked JBS issue only relates to heap dumps generated in case of OOM, I think we can edit it to more broadly support filename expansion to support `%t` for jcmd as well. 
>> 
>> Testing: 
>> - [x] Added test cases pass with all platforms (verified with a GHA job). 
>> - [x] Tier 1 passes with GHA. 
>> 
>> Looking forward to hearing your thoughts!
>> 
>> Thanks, 
>> Sonia
>
> Sonia Zaldana Calles 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 two additional commits since the last revision:
> 
>  - Merge branch 'openjdk:master' into JDK-8204681
>  - 8204681: Option to include timestamp in hprof filename

Hi folks, 

Thanks for your comments. I spent some time today prototyping how to reuse UL tag logic. Like @kevinjwalls mentioned, it becomes a bit tricky when the identifiers are more than one letter. Especially as it might lead to some unintended consequences. For example, a user can specify the file name `my%tinstance` and end up with a thread id (`ti` per UL tags) versus an intended timestamp. 

I didn’t want to spend too much time on making the 2-letter identifiers work but I made some small changes to use UL tags in general. Looks like [this](https://github.com/SoniaZaldana/jdk/commit/36cdef1a598761e5310da4e30c39e818f2ff1230).

Note the timestamp is diplayed in the following format: `2024-10-31T15:14:39.344-0400`

As far as the scope of this PR, I'm okay to implement it using UL tags (one or two letters) if we feel that will serve argument expansion in the long term. However, I still have the question about backwards compatibility. Would reviewers be receptive to changing `Arguments::copy_expand_pid` to `Arguments::copy_expand_arguments` and propagate that across all places where `Arguments::copy_expand_pid` as originally proposed or is there another preferred approach? 

Thanks for taking a look! 

Sonia

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

PR Comment: https://git.openjdk.org/jdk/pull/20568#issuecomment-2450656058