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

Thanks Thomas -
Good to get this going again 8-)

Not really against other substitutions being available if somebody is going to implement them. 8-)
(and explore the possible code sharing)

Timestamp is valuable as it's a commonly needed and hard to know ahead of time.
It seems useful that we do it in the JVM, as while I can call out to `date +%...` in a terminal or in a script, I have to think a fair amount (and generate a date string in a consistent way, and without spaces and slashes).


Can't see a problem with %h hostname being added later if there is a perceived need, if this change goes ahead just doing timetamps.

I was thinking about the changes being possible incrementally, but much seems to depend on how much work the author wants to take on at once.


For parsing a filename in a command, one-letter identifiers like %p, %t, and maybe %h etc... are unambiguous to parse.

Using the unified logging decorator set, we start using two characters or more. Things get ambiguous if we don't terminate the parsing somehow, and the user has to say something like "filename%(t)issue123.out" if they don't separate things with non alphabetic characters, e.g. "filename_%t_issue123.out".  I might prefer the latter, but not sure I can say everybody has to do it that way...



Now I reread the original issue:
8204681: Option to include timestamp in hprof filename
..so that was raised about -XX:HeapDumpOnOutOfMemoryError etc...

If this change is to add %t timestamp (or more) in jcmd output files, it should all be in a new issue like "Option to include timestamp in jcmd output filenames", and in future -XX:HeapDumpOnOutOfMemoryError etc could still be expanded to respect a timestamp etc... with 8204681.

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

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