cryostatio / cryostat-web

Web front-end for Cryostat: Secure JDK Flight Recorder management for containerized JVMs
https://cryostat.io/
Other
10 stars 20 forks source link

feat(graphql): recording metadata/labels handling #1227

Closed aali309 closed 4 months ago

aali309 commented 5 months ago

Welcome to Cryostat! 👋

Before contributing, make sure you have:

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main


Related to: 315 Depends on: 319

Description of the change:

This change adds allows a match expression example to be copied to the clipboard...

Motivation for the change:

This change is helpful because users may want to copy the example for easier use...

How to manually test:

  1. see https://github.com/cryostatio/cryostat3/pull/319
mergify[bot] commented 5 months ago

⚠️ The sha of the head commit of this PR conflicts with #1224. Mergify cannot evaluate rules on this PR. ⚠️

andrewazores commented 5 months ago

I tried to edit labels on active recordings and got this error:

image

And on Archives > All Targets:

image

cryostat_1            | Mar 20, 2024 4:23:20 PM io.quarkus.vertx.http.runtime.filters.accesslog.JBossLoggingAccessLogReceiver logMessage
cryostat_1            | INFO: 10.89.1.66 - - [20/Mar/2024:16:23:20 +0000] "POST /api/v2.2/graphql HTTP/1.1" 308 -
auth_1                | 10.89.1.66:46816 - ca68af46-5a6d-4b11-8cb4-7d9fe9bed780 - user [2024/03/20 16:23:20] localhost:8080 POST cryostat "/api/v2.2/graphql" HTTP/1.1 "Mozilla/5.0 (X11; Linux x86_64; rv:123.0) Gecko/20100101 Firefox/123.0" 308 0 0.002
cryostat_1            | Mar 20, 2024 4:23:20 PM io.smallrye.graphql.execution.ExecutionService execute
cryostat_1            | INFO: SRGQL011005: Payload In [ query PostRecordingMetadata($connectUrl: String, $recordingName: String, $labels: [Entry_String_StringInput]) { targetNodes(filter: { <<<<<<< grapql-updateMetadata2 target{ archivedRecordings(filter: { name: $recordingName }) { data { doPutMetadata(metadataInput:{labels: $labels }) { metadata { labels{ key value } } size archivedTime } } } } } }]
auth_1                | 10.89.1.66:46816 - c5dc82a0-ebab-4ae0-b48a-b344b82ff276 - user [2024/03/20 16:23:20] localhost:8080 POST cryostat "/api/v3/graphql" HTTP/1.1 "Mozilla/5.0 (X11; Linux x86_64; rv:123.0) Gecko/20100101 Firefox/123.0" 200 136 0.003
cryostat_1            | Mar 20, 2024 4:23:20 PM io.quarkus.vertx.http.runtime.filters.accesslog.JBossLoggingAccessLogReceiver logMessage
cryostat_1            | INFO: 10.89.1.66 - - [20/Mar/2024:16:23:20 +0000] "POST /api/v3/graphql HTTP/1.1" 200 136

On Archives > All Archives:

image

Screenshot_2024-03-20_12-24-49

aali309 commented 5 months ago

I think yesterday with the re-base, I missed a commit

andrewazores commented 5 months ago

Archives > All Archives > Edit Labels still expects to use an endpoint that we haven't reimplemented on the server:

POST http://localhost:8080/api/beta/fs/recordings/Vzxaji_a8E-j8pwLi9GH6QQLw8qEmc88EleBnWxQaJw%3D/compose-cryostat-1_test_20240320T180444Z.jfr/metadata/labels

Could you check on that and see if it's reasonable to make that just use the same GraphQL doPutMetadata instead?

aali309 commented 5 months ago

Archives > All Archives > Edit Labels still expects to use an endpoint that we haven't reimplemented on the server:

POST http://localhost:8080/api/beta/fs/recordings/Vzxaji_a8E-j8pwLi9GH6QQLw8qEmc88EleBnWxQaJw%3D/compose-cryostat-1_test_20240320T180444Z.jfr/metadata/labels

Could you check on that and see if it's reasonable to make that just use the same GraphQL doPutMetadata instead?

Looks good, I think. Please take a look.

andrewazores commented 5 months ago

Label editing for archived recordings looks like it's working well, but when I edit labels on an active recording on the Recordings > Active view, now I do get notifications appearing after I try to make changes but the view still doesn't seem to update properly until I navigate away or refresh.

andrewazores commented 5 months ago

After adding a label:

image

After navigating away and back:

image

Trying to remove this label:

image

andrewazores commented 5 months ago

What I see now is that the Archives > All Archives view updates when labels are edited, but it looks like it does it by doing a full refresh since I can see the whole table redraws an update. The Archives > All Targets, Recordings > Active, and Recordings > Archived views don't seem to update when labels are edited, except when refreshing or navigating away or back.

What this looks like to me is that the tables are either not listening for WebSocket notifications, or if they are, they are expecting a different format than what the server is sending. It should be that the server emits a WebSocket notification whenever metadata is updated on an active or archived recording, and that notification should contain enough information for the web-client to known which view this notification should apply to and what data to update in the view. It shouldn't be necessary to do a full refresh of the table by issuing an HTTP/GraphQL API request.

andrewazores commented 5 months ago

Looking good now on the Recordings view - both Active and Archived recordings seem to function perfectly for editing/updating labels, getting notifcations, etc.

On the Archives view, the Uploads tab also seems to be working perfectly. But the All Targets and All Archives views don't seem to let me edit labels. When I try it there is no error notification but also no update notification, the UI doesn't update, and if I navigate away and back I don't see the updated labels changed either.

Also, on any of the views/tabs, if I click on a label it should filter the list to show all the recordings which match the same set of selected labels. Instead, I just get an empty list.

image

image

aali309 commented 5 months ago

Looking into it.

aali309 commented 5 months ago

the Archives -> All Targets seem to work well with me.

image

even the All Archives

image

Clicking on any label however shows no recordings with the set of labels, looking to fix this.

andrewazores commented 5 months ago

I'll try to replicate it again. I did this after doing a lot of other clicking around on the two Recordings view tabs, so maybe I triggered some other bug that broke this part indirectly.

andrewazores commented 5 months ago

This time around All Targets and All Archives are working perfectly as expected. I guess there was some other bug that I triggered, but I don't know what it was or how to trigger it again. If we see it again we should try to figure out what it was and file a separate bug report to fix later.

Once the feature of "list filtering by clicking labels" is fixed then I'll come back around and do the code review. Happy to see this all turned out so well :-)

aali309 commented 5 months ago

looks good now

image

aali309 commented 5 months ago

@andrewazores Upon testing, I see that deleting archive recordings does not update the table until after navigating away and back or refresh. I'll ping you once its ready.

aali309 commented 5 months ago

@andrewazores Upon testing, I see that deleting archive recordings does not update the table until after navigating away and back or refresh. I'll ping you once its ready.

Looks good now.

andrewazores commented 5 months ago

One more very minor bug. On All Archives, after editing labels and hitting Save, the editor split panel closes and it looks like the view does a refresh (I see a brief flicker). On the other table views I can edit labels, hit Save, and see the updated data in the view while the editor split panel stays open.

aali309 commented 5 months ago

One more very minor bug. On All Archives, after editing labels and hitting Save, the editor split panel closes and it looks like the view does a refresh (I see a brief flicker). On the other table views I can edit labels, hit Save, and see the updated data in the view while the editor split panel stays open.

I thought this was how it was set from the beginning. I will look into it.

andrewazores commented 5 months ago

It may be, but since we're already here it would be worth fixing up too. It's also one that you've upgraded to using GraphQL, so I wouldn't be surprised if it was also an oddball and had this other bug in it.

aali309 commented 5 months ago

Looks good now

image

andrewazores commented 5 months ago

I think this also needs a yarn format:apply

aali309 commented 4 months ago

Ok @andrewazores, I think all views and tables look good with the doPutMetadata and doDelete.

andrewazores commented 4 months ago

Just in case, please don't delete your PR branch here yet. I might need to come back and reference it to make sure the merge into the base feature branch was successful.