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

[Task] Fine-grained replacement parameter for Start Recording Mutator #1492

Closed
tthvo opened this issue May 16, 2023 · 0 comments · Fixed by #1587
Closed

[Task] Fine-grained replacement parameter for Start Recording Mutator #1492

tthvo opened this issue May 16, 2023 · 0 comments · Fixed by #1587
Assignees
Labels
feat New feature or request good first issue Good for newcomers

Comments

@tthvo
Copy link
Member

tthvo commented May 16, 2023

Describe the feature

I think the best thing to do here is actually just ignore the request on any targets that already have such a recording running, since starting a new parallel active recording in the same running state doesn't actually capture any additional data. If the recording doesn't exist, it should be created and started. If it exists but has been stopped, it should be restarted. If it is already running just do nothing.

This I think probably can and should be done server-side.

https://github.com/cryostatio/cryostat/blob/2b0cd5adacf8f928f8666f36663b0a5b31e0e76b/src/main/java/io/cryostat/net/web/http/api/v2/graph/StartRecordingOnTargetMutator.java#L181

https://github.com/cryostatio/cryostat/blob/2b0cd5adacf8f928f8666f36663b0a5b31e0e76b/src/main/java/io/cryostat/recordings/RecordingTargetHelper.java#L157

Currently, when requesting to start a recording, the client can use a query parameter or GraphQL argument restart to ask the server to close and re-open any recordings that already exist with the same name as in the new request. If this parameter is not provided or is false then the server will either create the new recording or will fail with an exception. If the parameter is true then the server will either create the new recording, or will stop the existing recording and start a new one with the other request parameters (event template, maxAge/maxSize settings, etc.).

I think this behaviour makes sense to retain, and it is already part of published API so it would be nice to be able to enhance this while keeping the same request format. I would propose that the query parameter ?restart=[true|false] be kept but documented as deprecated, and a new replacement like ?replace=[always,stopped,never] introduced. ?replace=always would behave the same as ?restart=true, ?replace=never would behave the same as ?restart=false, and ?replace=stopped would implement the quoted behaviour. If replace is not specified then it should behave like ?replace=never, so that this also maintains consistency and compatibility with the existing ?restart=false default behaviour.

Originally posted by @andrewazores in cryostatio/cryostat-web#1035 (comment)

@tthvo tthvo added good first issue Good for newcomers feat New feature or request labels May 16, 2023
@tthvo tthvo moved this to Todo in 2.4.0 release May 16, 2023
@andrewazores andrewazores changed the title [Request] Fine-grained replacement parameter for Start Recording Mutator [Task] Fine-grained replacement parameter for Start Recording Mutator May 19, 2023
@andrewazores andrewazores moved this from Todo to Backlog in 2.4.0 release May 23, 2023
@aali309 aali309 self-assigned this Jul 5, 2023
@github-project-automation github-project-automation bot moved this from Backlog to Done in 2.4.0 release Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request good first issue Good for newcomers
Projects
No open projects
Status: Done
2 participants