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. This is a very good thought. There were people that already thought about useful things to have in a log. This is very similar. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20568#issuecomment-2344045803
