On Wed, 21 Aug 2024 09:54:08 GMT, Thomas Stuefe <stu...@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 > > I think this could be very useful, but it needs more preparation and > decisions. Possibly a CSR. > > - copy_expand_xxx is used in many places. While I think all of these places > would benefit from more expansions than just %p, there is a potential > backward compatibility issue if clients use %t for whatever reason today > - Do we want the time of the dump or the JVM start? If the JVM runs for a > week, then produces a JFR file, should the file be named by the JVM start > date? I think in most cases the *current* time makes more sense > - Do we want the printout as a human-readable date or as a numeric timestamp? > Both makes sense depending on the post-processing clients want to do. > - Do we want to improve this function further, potentially adding more > replacement options? > > One possible way to solve this: > - use different characters for timestamp (number) and datetime (human > readable date) > - use always the current time > - If we want to add further replacements: > - come up with a new replacement character that does not clash with libc > sprintf (IMHO using percent was not a good idea in the first place). E.g. `$` > - Add a new switch to guard this new replacement logic. By default off. If > on, the contract is that any character following a `$` may be either now or > in the future replaced with something different. Client must not use `$` as a > normal character. > - We probably should remove all non-matching `$` from the input. > - The first replacements could be: `$p` for pid, `$t` for timestamp > (numeric), `$d` for datetime > - later replacements can be added later. Since we guard the new feature > with a switch and forbid the use of `$`, we are then free to do so without > breaking backward compatibility. > > I would like to hear @dholmes-ora take on this. > > We had a similar system at SAP in our proprietary JVM, which was really > useful, so I like this idea in general. I don't object (don't really have strong views) on adding this functionality, but as @tstuefe notes there are a few things to consider. I'm not really averse to using the `%` character precisely because it is commonly identified as a format specifier - and I think `$` would be very problematic due to shell issues. At the risk of seeking the perfect instead of just doing what is immediately "good enough" we might also look at the unified logging decorators as potential formats: Available log decorators: time (t), utctime (utc), uptime (u), timemillis (tm), uptimemillis (um), timenanos (tn), uptimenanos (un), hostname (hn), pid (p), tid (ti), level (l), tags (tg) and it may also allow for some code sharing. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20568#issuecomment-2306234386
