-
Notifications
You must be signed in to change notification settings - Fork 10
feat(diagnostics: Add thread dumps endpoint and storage #894
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: main
Are you sure you want to change the base?
Conversation
log.error(e); | ||
return null; | ||
} | ||
}) |
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.
A .filter()
here to remove null
elements would probably be good.
|
||
private ThreadDump convertObject(S3Object object) throws Exception { | ||
var req = GetObjectTaggingRequest.builder().bucket(bucket).key(object.key()).build(); | ||
var tagging = storage.getObjectTagging(req); |
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.
This will need some reworking to match #927 - we cannot always rely on object Tagging. Object Metadata seems like a more widely supported feature and probably suitable here since it doesn't seem like thread dumps need mutable metadata, but in that PR I also laid out a way to use a separate bucket of files to store metadata as JSON as well as a last resort option.
} | ||
|
||
private Tagging createTagging(String jvmId, String uuid) { | ||
var map = Map.of("jvmId", jvmId, "uuid", uuid); |
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 don't think the uuid
needs to be included as tagging/metadata, since it's also used as the object's key already.
.findFirst() | ||
.orElseThrow(); | ||
// content, jvmid, downloadurl, uuid | ||
return new ThreadDump(getThreadDumpContent(uuid), jvmId, downloadUrl(jvmId, uuid), uuid); |
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.
Since individual dumps are relatively large, I think it might be best to treat these more like how we do JFR files, and not embed the actual file content directly in listing/query operations - only when a specific file (dump) is directly requested.
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.
Also, this current structure leads to an initial S3 query to list N objects in the bucket, and then N follow-up sequential queries to retrieve all of their content and join that into the final List<ThreadDump>
to return to the client, so if N is large and/or if the connection to S3 storage is slow, this query operation will also be quite slow.
So I think having one listing query that just provides metadata about the available dump files (UUID, JVM ID, maybe the date it was created, filesize, etc.) and then a separate operation to actually download the file content given a JVMID/UUID pair would be best. The UI should also be adjusted accordingly to only render a list/table of the metadata first, and require the user to perform some interaction to trigger a download and display of the full dump content.
} | ||
|
||
public String getThreadDumpContent(String uuid) throws IOException { | ||
InputStream is = getModel(uuid); |
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.
This stream should be closed
@RolesAllowed("write") | ||
@Blocking | ||
@POST | ||
public String threadDump( | ||
HttpServerResponse response, @RestPath long targetId, @RestQuery String format) { | ||
log.trace("Creating new thread dump request"); | ||
log.trace("Creating new thread dump request for target: " + targetId); |
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.
Best to use the log.tracev("message {0} {1}", obj1, obj2)
format specifier style rather than direct string concatenation. Using concatenation will result in an eager .toString()
conversion being called on the operands, even if the message is logged at a lower level than currently configured.
return helper.getThreadDumps(bucket); | ||
log.warn("Fetching thread dumps for target: " + targetId); | ||
log.warn("Thread dumps: " + helper.getThreadDumps(targetId)); | ||
log.warn("Storage bucket: " + bucket); |
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.
Similar for .warnv()
here, especially since there is the getThreadDumps()
call which will be pretty expensive for a simple logging call.
Related to: #135
Initial implementation of thread dumps endpoint and storage setup/interaction
Opening as Draft until frontend is done since this will probably end up needing adjustments as that gets implemented.
TODO: Refactor (probably into a ThreadDumpHelper class like RecordingHelper)