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.

Sign up for GitHub

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

Jump to bottom

feat(api): mutation operations #175

Merged
merged 48 commits into from
Sep 19, 2023
Merged

feat(api): mutation operations #175

merged 48 commits into from
Sep 19, 2023

Conversation

andrewazores
Copy link
Member

@andrewazores andrewazores commented Jul 27, 2023 •
edited
Loading

@andrewazores andrewazores linked an issue Jul 27, 2023 that may be closed by this pull request
[Epic] Two-way communications protocol #124
Open
3 tasks
@github-actions
Copy link

Hi @andrewazores! Add at least one of the required labels to this PR

Required labels are : chore,ci,cleanup,docs,feat,fix,perf,refactor,style,test

@andrewazores andrewazores added the feat New feature or request label Jul 27, 2023
@mergify mergify bot added the safe-to-test label Jul 27, 2023
@andrewazores andrewazores changed the title feat(api): mutation operations WIP feat(api): mutation operations Jul 31, 2023
@andrewazores andrewazores force-pushed the 124-epic-two-way-communications-protocol branch from 0a60a1f to 7b774d9 Compare August 4, 2023 19:06
@andrewazores
fix license header
08a200f
@andrewazores
end paths with / so as to not match by prefix
1a2acaa
@andrewazores
refactor: extract methods
18f3742
@andrewazores
add utility for extracting ID from path
6577734
@andrewazores
fixup! add utility for extracting ID from path
8789208
@andrewazores
add handling for stopping recording
7b62753
@andrewazores
add handling for closing (deleting) recording
b8d10b8
@andrewazores
fixup! add handling for closing (deleting) recording
15195e7
@andrewazores
fixup! add handling for stopping recording
8cc9a87
@andrewazores
only allow GET requests if write-operations are not enabled
e4df526
@andrewazores
extract HTTP response body length constants
0bcaf25
@andrewazores
cleanup
5fc71f9
@andrewazores
feat(api): enable dynamic JFR stop, delete (#176)
6154aa8 
* end paths with / so as to not match by prefix

* refactor: extract methods

* add utility for extracting ID from path

* add handling for stopping recording

* add handling for closing (deleting) recording

* only allow GET requests if write-operations are not enabled

* extract HTTP response body length constants
@andrewazores
use longs for IDs
86a2c02
@andrewazores
feat(api): implement GET /recordings/:id for streaming files
abcb2b9
@andrewazores
refactor to use constants
42ecb61
@andrewazores
feat(api): enable dynamic JFR start (#165)
06a8cc1
@andrewazores
fix license header
8039907
@andrewazores
feat(api): enable dynamic JFR stop, delete (#176)
82002e8 
* end paths with / so as to not match by prefix

* refactor: extract methods

* add utility for extracting ID from path

* add handling for stopping recording

* add handling for closing (deleting) recording

* only allow GET requests if write-operations are not enabled

* extract HTTP response body length constants
@andrewazores andrewazores force-pushed the 124-epic-two-way-communications-protocol branch from 7c80d6c to 82002e8 Compare August 14, 2023 19:13
@andrewazores andrewazores marked this pull request as draft August 14, 2023 19:19
@andrewazores andrewazores force-pushed the 124-epic-two-way-communications-protocol branch from 3df2ec1 to 05ead99 Compare August 14, 2023 19:27
@andrewazores andrewazores mentioned this pull request Sep 13, 2023
Merged
11 tasks
ebaron
ebaron reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

The writes-enabled property looks fine to me. We just need to make sure users are aware of it through documentation. Since this is only gating new functionality for the agent, I don't think it's overly disruptive from a UX perspective.

src/main/java/io/cryostat/agent/remote/RecordingsContext.java Outdated Show resolved Hide resolved
@andrewazores andrewazores force-pushed the 124-epic-two-way-communications-protocol branch from 1cc7bdc to baa3e37 Compare September 13, 2023 19:05
@mwangggg mwangggg self-requested a review September 14, 2023 18:24
mwangggg
mwangggg previously approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@mwangggg mwangggg left a comment

Choose a reason for hiding this comment

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

looks good to me 👍

@andrewazores
Copy link
Member Author

@mwangggg do the latest changes make sense to you?

@andrewazores
Copy link
Member Author

@mwangggg ping

@mwangggg
Copy link
Member

mwangggg commented Sep 19, 2023 •
edited
Loading

Sorry I don't seem to be getting notifications when my username is pinged in comments- I just updated my email notification settings so hopefully I can be faster in the future when replying. Regarding the updates, I ran into an error when I tried to stop the cryostat-agent recording in quarkus-test-agent-1. The recording is updated to a STOPPED status as expected, but I'm not sure what's causing the error (the NoSuchElementException):
Screenshot from 2023-09-18 23-47-03
The cryostat logs:

i://cryostat:9097/jmxrmi"}}
Sep 19, 2023 3:46:39 AM io.cryostat.core.log.Logger error
SEVERE: HTTP 500: No value present
io.vertx.ext.web.handler.HttpException: Internal Server Error
Caused by: java.util.NoSuchElementException: No value present
	at java.base/java.util.Optional.get(Optional.java:143)
	at io.cryostat.recordings.RecordingTargetHelper.lambda$stopRecording$4(RecordingTargetHelper.java:286)
	at io.cryostat.net.TargetConnectionManager.executeConnectedTask(TargetConnectionManager.java:151)
	at io.cryostat.recordings.RecordingTargetHelper.stopRecording(RecordingTargetHelper.java:267)
	at io.cryostat.recordings.RecordingTargetHelper.stopRecording(RecordingTargetHelper.java:260)
	at io.cryostat.net.web.http.api.v1.TargetRecordingPatchStop.handle(TargetRecordingPatchStop.java:40)
	at io.cryostat.net.web.http.api.v1.TargetRecordingPatchHandler.handleAuthenticated(TargetRecordingPatchHandler.java:105)
	at io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler.handle(AbstractAuthenticatedRequestHandler.java:81)
	at io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler.handle(AbstractAuthenticatedRequestHandler.java:51)
	at io.vertx.ext.web.impl.BlockingHandlerDecorator.lambda$handle$0(BlockingHandlerDecorator.java:48)
	at io.vertx.core.impl.ContextBase.lambda$null$0(ContextBase.java:137)
	at io.vertx.core.impl.ContextInternal.dispatch(ContextInternal.java:264)
	at io.vertx.core.impl.ContextBase.lambda$executeBlocking$1(ContextBase.java:135)
	at io.vertx.core.impl.TaskQueue.run(TaskQueue.java:76)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.base/java.lang.Thread.run(Thread.java:833)

@andrewazores
Copy link
Member Author

I'll take a look at that over at https://github.com/cryostatio/cryostat/pull/1608 - that target uses the Agent for Discovery only, with a JMX URL for all actual JFR communications, so the recording stop action isn't going through any Agent code here.

mwangggg
mwangggg approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@mwangggg mwangggg left a comment

Choose a reason for hiding this comment

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

looks good to me

@andrewazores andrewazores merged commit 7132572 into main Sep 19, 2023
9 checks passed
@andrewazores andrewazores deleted the 124-epic-two-way-communications-protocol branch September 19, 2023 14:43
@andrewazores andrewazores mentioned this pull request Sep 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebaron ebaron ebaron left review comments

@mwangggg mwangggg mwangggg approved these changes

Assignees
No one assigned
Labels
feat New feature or request safe-to-test
Projects
No open projects
2.4.0 release
Status: Done
3.0.0 release
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Epic] Two-way communications protocol
3 participants
@andrewazores @mwangggg @ebaron