-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8357828: Add a timestamp to jcmd diagnostic commands #27368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back Domest0s! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@Domest0s The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
| } | ||
|
|
||
| void ThreadDumpToFileDCmd::execute(DCmdSource source, TRAPS) { | ||
| print_local_time(output()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you didn't mean to print a time stamp here as the output already has a timestamp. Also for -format=json and sending the output to stdout, then it will be unparsable if a timestamp appears before the JSON object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Alan. That's right.
My mistake. I declared the intention to not have timestamps for commands with a throwaway stdout. This also includes "Thread.dump_to_file" command but then mistakenly added print_local_time() in here.
|
Internal testing tier 1-5 did not show any issues. |
|
This was raised and discussed somewhere else not that long ago, but I don't recall where and it fizzled out as I recall. But I will re-raise the issues - what form should this timestamp take? Shouldn't it be configurable to allow matching with what may be presented by Unified Logging e.g. VM uptime rather than wall-clock time? The value of a timestamp on dmcd output is to allow it to be placed in chronological order relative to other output - so you need to be able to match the "timestamp" of that other output. And rather than add timestamp printing code to each dcmd it would make more sense to me to have a global dcmd flag that says "print a timestamp" (with a given format). That way users opt-in and we don't have to remember to add this for new Dcmds. |
|
The Thread.print dcmd already uses the yyyy-MM-dd HH:mm:ss format but yes, having all timestamps in a consistent format and time zone is useful for log comparisons. I'd second the global dcmd flag suggestion which would allow the option of the timestamp(on or off). Some commands should have timestamp on by default though. Those where output is sensitive to the time that the command was issued. e.g. Looking at such data output in isolation with the absence of timestamps makes it far less valuable IMO, especially when one wants to compare it to application logs, GC logs etc. |
|
I think it should be clearer what the benefit is, who the users are. In a remote support situation where the the user is gathering info, then timestamps are good. An optional global timestamp setting could be in the Jcmd launcher, which would mean it doesn't have to be implemented in each native diagnostic command impl. jcmd -T PID command... (or whatever option) That might be straightforward, but then you can get double timestamps if the jcmd implementation already prints one (so do you remove the timestamp from Thread.print if the jcmd launcher starts printing it?) Also complicated for some to commands enable a timestamp by default, the jcmd launcher does not know anything about command implementations. Do we print a timestamp if the command is not recognised? Lots of questions. 8-) |
|
Speaking of having "print a timestamp" as a global flag. Placing be the flag early for the launcher, before
Is there any issues to that? On the other hand, I see dcmd implements a special rule to handle "-h", "-help", and "--help" flags placed at the end of |
|
Hi, that sounds promising. Passing something on to the native impl is interesting - then, if the jcmd launcher prints pid:, then timestamp on a newline, can Thread.print find out a timestamp is already printed, and not duplicate it? 3975026: Or, should Thread.print stop printing a timestamp, and rely on the launcher to do it. That would avoid that communication, but would mean we have to default to timestamps ON, or Thread.print output format will change (which is not completely illegal, but will no doubt surprise some people/scripts). Or, have the two sides communicate and Thread.print prints the timestamp, if it has not been shown already. Nothing there is as simple as we'd like. 8-) |
|
It might be useful for @larry-cable to comment on this too. I think he's exploring some of these command serving up JSON rather than plain text. Any "infrastructure" that print a plain text timestamp could mess that up. |
|
Piling on... I think this feature would be useful. I wished for this more often than not. I think the printing should be done on the server side, not on the client side (be it a I would prefer a timestamp in the UL "time" decoration format: Full ISO-8601, including timezone offset. See https://openjdk.org/jeps/158 . That would also keep in line with gc logs etc. I am not sure we need a specific option to disable or enable this. Who would want to disable this feature? |
|
Mailing list message from Laurence Cable on serviceability-dev: On 9/23/25 2:59 AM, Thomas Stuefe wrote:
anyone who is parsing the output of jcmd's programmatically ... since |
@tstuefe Do you think we would need a CSR for this kind of change? |
yeah, true @Domest0s Thinking this a bit further. We may have the same discussion in the future whenever we do fundamental changes to jcmd output. As an arbitrary example, let's say someone wants to report the jcmd runtime back at the end of every command (I recently needed that, but abandoned the PR). E.g. a trailing "321ms". Or some other statistics or process specs. Do we want a new diagnostic switch every time? That could get confusing quickly, and these switches need testing. How about a single "enable jcmd legacy output format" setting. By default on. Behind this setting we guard all future changes that severely change the output of commands. Anyone wanting a stable output would define that setting. And how about making this switch a standard jcmd option, not a JVM setting? Typically, this decision is made by the analyst running jcmd. That is the person wanting to post-process the output. A JVM switch means the analyst has to restart the JVM with new command-line parameters. That is uncomfortable, and the analyst may even be able to do that (often can't restart client production servers; maybe not even change VM parameters). Opinions? |
|
Mailing list message from Laurence Cable on serviceability-dev: see inline... On 9/24/25 2:37 AM, Thomas Stuefe wrote:
thanks!
cheers!
agreed!
agreed
agreed
makes sense, I have a (much) larger dream for jcmd etc that I want to 1) add Unix Domain Socket support to the (sun) enbedded httpserver with json as a payload format such additions would be much simpler...(in |
|
@Domest0s Let's see if everyone agrees on this. My proposal would entail
How complex would this be? There are two approaches to this. You can either create a new attach listener protocol (we already have ATTACH_API_V2 vs ATTACH_API_V1, so the precedence is there). Or, you can expand the jcmd parsing (see Not sure whether this would be complex. I understand if this is expanding the scope of your PR too much, in which case I think you should add a new JVM command line option that preserves legacy jcmd output format. I think you need a CSR in both cases. Not exactly because compatibility would be broken (the jcmd output should stay the same unless this option is explicitly enabled), but because it makes sense to think a bit about the format of this new jcmd option or this new command line flag, and to make the new command line flag product. Just my 5 cents, let's see what others think first |
That sounds interesting, and useful. |
ATTACH_API_V2 supports "options". |
|
On 9/24/25 12:50 PM, Alex Menkov wrote:
*alexmenkov* left a comment (openjdk/jdk#27368)
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/27368*issuecomment-3330432466__;Iw!!ACWV5N9M2RV99hQ!L8Ino0e59huBM_uRbUgs_2H_F9z8ErphlRCLbOg46qe85WuOwQe9PPmlOD1eOCuSVjCV2Xyj6sI_YJnNWYmYB9_KGw$>
My proposal would entail
* adding a new standard option to jcmd
* somehow funneling that option to the JVM
* the option should be optional, so that:
* old jcmd still works with new JVMs (and produces legacy jcmd output)
* new jcmd still works with old JVMs (and produces legacy jcmd output)
How complex would this be? There are two approaches to this. You
can either create a new attach listener protocol (we already have
ATTACH_API_V2 vs ATTACH_API_V1, so the precedence is there). Or,
you can expand the jcmd parsing (see |jcmd(AttachOperation* op,
attachStream* out)| in attachListener.cpp).
ATTACH_API_V2 supports "options".
Client (jcmd) detects options supported by the target VM ("getversion
options" command) and can set option values in attach command request.
Currently the only supported option is "streaming" (it allows turn off
streaming output).
The option is needed only for tests and needs to work for all attach
tools (jcmd, jstack, etc.), so it can be set by specifying java
property launching attach tool (like |jcmd
-J-Djdk.attach.allowStreamingOutput=false PID command|)
sounds like a plan!
… —
Reply to this email directly, view it on GitHub
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/27368*issuecomment-3330432466__;Iw!!ACWV5N9M2RV99hQ!L8Ino0e59huBM_uRbUgs_2H_F9z8ErphlRCLbOg46qe85WuOwQe9PPmlOD1eOCuSVjCV2Xyj6sI_YJnNWYmYB9_KGw$>,
or unsubscribe
<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67SGRSQRLDJZAM3LPRT3ULYW5AVCNFSM6AAAAACG4ANNIKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGMZQGQZTENBWGY__;!!ACWV5N9M2RV99hQ!L8Ino0e59huBM_uRbUgs_2H_F9z8ErphlRCLbOg46qe85WuOwQe9PPmlOD1eOCuSVjCV2Xyj6sI_YJnNWYk7VM8-7g$>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--------------oR0JTaz5Fop00jaWhAnzYKGf
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit
<!DOCTYPE html><html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<br>
<br>
<div class="moz-cite-prefix">On 9/24/25 12:50 PM, Alex Menkov wrote:<br>
</div>
<blockquote type="cite" ***@***.***">
<div style="display: flex; flex-wrap: wrap; white-space: pre-wrap; align-items: center; "><img height="20" width="20" style="border-radius:50%; margin-right: 4px;" decoding="async" src="https://avatars.githubusercontent.com/u/69548902?s=20&v=4" moz-do-not-send="true"><strong>alexmenkov</strong> left a comment <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/27368*issuecomment-3330432466__;Iw!!ACWV5N9M2RV99hQ!L8Ino0e59huBM_uRbUgs_2H_F9z8ErphlRCLbOg46qe85WuOwQe9PPmlOD1eOCuSVjCV2Xyj6sI_YJnNWYmYB9_KGw$" moz-do-not-send="true">(openjdk/jdk#27368)</a></div>
<blockquote>
<p dir="auto">My proposal would entail<br>
* adding a new standard option to jcmd<br>
* somehow funneling that option to the JVM<br>
* the option should be optional, so that:<br>
* old jcmd still works with new JVMs (and produces legacy jcmd
output)<br>
* new jcmd still works with old JVMs (and produces legacy jcmd
output)<br>
How complex would this be? There are two approaches to this.
You can either create a new attach listener protocol (we
already have ATTACH_API_V2 vs ATTACH_API_V1, so the precedence
is there). Or, you can expand the jcmd parsing (see <code class="notranslate">jcmd(AttachOperation* op, attachStream*
out)</code> in attachListener.cpp).</p>
</blockquote>
<p dir="auto">ATTACH_API_V2 supports "options".<br>
Client (jcmd) detects options supported by the target VM
("getversion options" command) and can set option values in
attach command request.<br>
Currently the only supported option is "streaming" (it allows
turn off streaming output).<br>
The option is needed only for tests and needs to work for all
attach tools (jcmd, jstack, etc.), so it can be set by
specifying java property launching attach tool (like <code class="notranslate">jcmd
-J-Djdk.attach.allowStreamingOutput=false PID command</code>)</p>
</blockquote>
<br>
sounds like a plan!<br>
<br>
<blockquote type="cite" ***@***.***">
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br>
Reply to this email directly, <a href="https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/27368*issuecomment-3330432466__;Iw!!ACWV5N9M2RV99hQ!L8Ino0e59huBM_uRbUgs_2H_F9z8ErphlRCLbOg46qe85WuOwQe9PPmlOD1eOCuSVjCV2Xyj6sI_YJnNWYmYB9_KGw$" moz-do-not-send="true">view it on GitHub</a>, or <a href="https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/ANTA67SGRSQRLDJZAM3LPRT3ULYW5AVCNFSM6AAAAACG4ANNIKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGMZQGQZTENBWGY__;!!ACWV5N9M2RV99hQ!L8Ino0e59huBM_uRbUgs_2H_F9z8ErphlRCLbOg46qe85WuOwQe9PPmlOD1eOCuSVjCV2Xyj6sI_YJnNWYk7VM8-7g$" moz-do-not-send="true">unsubscribe</a>.<br>
You are receiving this because you were mentioned.<img src="https://github.com/notifications/beacon/ANTA67WKNASEIPXUMLRQ5RD3ULYW5A5CNFSM6AAAAACG4ANNIKWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTWGQJO5E.gif" height="1" width="1" alt="" moz-do-not-send="true"><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message
ID: <span><openjdk/jdk/pull/27368/c3330432466</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
***@***.***": "http://schema.org",
***@***.***": "EmailMessage",
"potentialAction": {
***@***.***": "ViewAction",
"target": "#27368 (comment)",
"url": "#27368 (comment)",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
***@***.***": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>
</blockquote>
<br>
</body>
</html>
--------------oR0JTaz5Fop00jaWhAnzYKGf--
|
The output from several commands have changed over many releases/years, the output from Thread.print in particular. AFAIK, none of these changes has a CSR, there were only CSRs when new commands were added. That said, if there is some evidence that the output is being parsed then there is a compatibility concern so creating a CSR and RN would prudent. |
|
Mailing list message from Laurence Cable on serviceability-dev: On 9/25/25 12:00 AM, Alan Bateman wrote:
I am personally aware of at least one cloud svc that parses jcmd o/p |
Agreed. I will be working on the proposal. Thank you. |
|
Mailing list message from Laurence Cable on serviceability-dev: at some point (soon) we should also define a (common) JSON envelope IMO it should potentially include: - jcmd "name" all jcmds supporting JSON o/p should wrap their content in this std the motivation for this is that since emitting JSON is primarily - Larry On 10/7/25 1:14 PM, Ivan Bereziuk wrote: |
jcmdprovides great diagnostics but many commands lack a timestamp in their output.Adding a timestamp to the output of some would add value for those debugging JVM data.
Some diagnostic commands already provide timestamps. For example
Thread.printalready prints one of "yyyy-MM-dd HH:mm:ss" format.Adding timestamps to all diagnostic
jcmdcommands with a non-throw-away STDOUT.The exceptions are:
VM.uptime- command run with-dateargument will also print a timestamp;VM.system_properties- already lists timestampThread.dump_to_file- the content dumped to file already has a timestamp;VM.versionProgress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27368/head:pull/27368$ git checkout pull/27368Update a local copy of the PR:
$ git checkout pull/27368$ git pull https://git.openjdk.org/jdk.git pull/27368/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27368View PR using the GUI difftool:
$ git pr show -t 27368Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27368.diff
Using Webrev
Link to Webrev Comment