Skip to content
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

feat(recordings): Add recording metadata labels #835

Merged
merged 49 commits into from
Mar 9, 2022

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Feb 17, 2022

Related #709
Depends on cryostatio/cryostat-web#383

Adds a patch handler and helper functions to optionally add labels when starting a recording on a target. The patch handler is used for editing labels for existing active or archived recordings.

@jan-law jan-law added the feat New feature or request label Feb 17, 2022
@jan-law jan-law force-pushed the recording-labels branch 3 times, most recently from 3afbb7c to 41cee2c Compare March 1, 2022 19:27
@jan-law jan-law marked this pull request as ready for review March 1, 2022 19:45
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall, thanks for all this! Other than the small details I've left comments about, I have one higher-level internal implementation detail concern here. It seems that the API is defined to accept the labels as a string, but that string must be a JSON object/map. But, internally, the labels are passed around as the raw String, and when included in an API response (as a serialized recording descriptor), the labels field is actually again simply a String that has contents that appear to be JSON - but it will be wrapped in double quotes in the response as a String, not an actual JSON object.

I think it would be best to parse the client's request body as JSON right away in the handlers/manager, expecting it to be mappable to a Java Map<String, String>. If it isn't then that is a 400 response. If it does, then the labels object that is passed around internally should be the Map, and this is also what should be kept as a field reference in the serializable recording descriptor. This can still be serialized to JSON for storage on disk, and when included as an API response as a serialized recording descriptor, it will be nicely embedded as a JSON object/map rather than a String that looks like further encoded JSON.

@jan-law jan-law force-pushed the recording-labels branch 2 times, most recently from 28f8bce to f12e311 Compare March 7, 2022 19:16
@andrewazores
Copy link
Member

Code looks good. Could you update HTTP_API.md?

@andrewazores
Copy link
Member

rebase please

@jan-law
Copy link
Contributor Author

jan-law commented Mar 9, 2022

Latest build failure caused by No recording data available in ArchivedReportJwtDownloadIT. This test is passing locally.

testDownloadRecordingUsingJwt  Time elapsed: 11.602 s  <<< ERROR!
java.util.concurrent.ExecutionException: java.lang.Exception: HTTP 500
	at itest.ArchivedReportJwtDownloadIT.testDownloadRecordingUsingJwt(ArchivedReportJwtDownloadIT.java:78)
Caused by: java.lang.Exception: HTTP 500
INFO: (10.0.2.100:58684): PATCH /api/v1/targets//service:jmx:rmi:%2F%2F%2Fjndi%2Frmi:%2F%2Fcryostat-itests:9091%2Fjmxrmi/recordings/someRecording 204 241ms
Mar 09, 2022 9:09:52 PM io.cryostat.core.log.Logger error
SEVERE: HTTP 500: io.cryostat.recordings.EmptyRecordingException: java.io.IOException: java.io.IOException: No recording data available
io.vertx.ext.web.handler.impl.HttpStatusException: Internal Server Error
Caused by: java.util.concurrent.ExecutionException: io.cryostat.recordings.EmptyRecordingException: java.io.IOException: java.io.IOException: No recording data available

@andrewazores andrewazores merged commit ff9af22 into cryostatio:main Mar 9, 2022
@jan-law jan-law deleted the recording-labels branch March 9, 2022 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants