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

[Kevin Walls]

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

A few notes, without reviewing the code further today, see if these all make sense and are settled:

"%t" expanded to timestamp seems simple enough (I think % is not controversial, extending from our existing use of %p)


But yes what is a timestamp?  There could be debate on the format.
The original issue came from a user request, but it was not fully specified, just a request for an automatic way of putting file creation time in an output filename.

So should we have %t and %d ? Or adopt all the decorator options from unified logging?  To me those seem extreme: if I just want output files to say when they were made, I don't really need options, or nanoseconds, and would be quite happy with the ostream.cpp style of "%t => YYYY-MM-DD_HH-MM-SS".


Sonia your points above:
(I haven't looked in detail at how this affects copy_expand_arguments, but if we can settle the jcmd options then that can follow...)

1. -filepattern
Would really like to avoid adding a new jcmd option, in all the diagnostic commands that take an output filename, and just go with a filename that respects some substitutions.

2
%t and %p are "unique enough" that we don't need "%TIMESTAMP" ?
We don't need to permit creating "my%perfect%test%file" without using "%%" to get a literal % included?

3.
-XX:+AllowFileNamePattern 
I would think at this step, we get our jcmds that take an output FILE to respect the %t timestamp.  There may be other XX options that take filenames which could be considered in future.

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

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