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, 

Sorry about the late reply. Just got back in from PTO. 

I’m glad to see that there is some consensus that diagnostic commands should ideally support timestamp expansion for output filenames (with the possibility of adding more replacement options in the future). 

Thomas raised the issue about backwards compatibility issues with `Arguments::copy_expand_arguments`. We’ve been discussing some ways to address this: 

1. Introduce a new parameter for jcmd that supports filename expansion (`-filepattern` or something along those lines.) We would then need to sort out ignoring `-filename` if a filename pattern is provided. 
2. Replacing `%t` with a pattern that is very unlikely to have been used and expanded by accident (`%TIMESTAMP`, etc). 
3. A HotSpot flag (`-XX:+AllowFileNamePattern`) to accept these patterns in multiple places in the JVM. 

Option 2 is definitely the most straightforward to implement but it’s also not the cleanest. There is always a slight risk that someone’s configuration might break. 

I’d really like to find an actionable consensus on what would be best. Open to explore other options if anyone has any better ideas. 

Aside from that, I have noted the feedback about using current time vs start time and the `YYYY-MM-DD_HH-MM-SS` format. I will be making the appropriate changes once we’ve settled the other stuff. 

Thanks!

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

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