feat(archives): Redesign "All Archives" view

[hareetd]

Fixes #400
Fixes #398

[hareetd]

Adding a query for all archived recordings as you suggested and using it to populate both the uploaded and target-specific archived recordings tables solves the issue of authentication-related Exceptions occurring. The current implementation of the ArchivedRecordingsTable populates the table by fetching the targetNodes.recordings.archived data from the backend. This causes the aforementioned Exceptions to occur because the RecordingsFetcher also creates a target connection to fetch the targetNodes.recordings.active data at the same time.

I'm also thinking of adding new notifications for when targets are added/deleted in order to keep the targets list up-to-date.

[andrewazores]

The current implementation of the ArchivedRecordingsTable populates the table by fetching the targetNodes.recordings.archived data from the backend. This causes the aforementioned Exceptions to occur because the RecordingsFetcher also creates a target connection to fetch the targetNodes.recordings.active data at the same time.

Nice, that makes sense. I wonder if there is some better way to structure the implementation of that query so that it only opens the target connection when actually needed, ie querying active recordings.

I'm also thinking of adding new notifications for when targets are added/deleted in order to keep the targets list up-to-date.

Is this something different than https://github.com/cryostatio/cryostat/blob/ed9ff7e2d13da4d6c1d51a3325098e4169845295/src/main/java/io/cryostat/platform/internal/MergingPlatformClient.java#L70 ?

[hareetd]

Nice, that makes sense. I wonder if there is some better way to structure the implementation of that query so that it only opens the target connection when actually needed, ie querying active recordings.

I can look into this in a separate PR, for now the All Archives view can use the new query.

Is this something different than https://github.com/cryostatio/cryostat/blob/ed9ff7e2d13da4d6c1d51a3325098e4169845295/src/main/java/io/cryostat/platform/internal/MergingPlatformClient.java#L70 ?

Ah, just a notification for target deletion then?

[hareetd]

Never mind, took a look at the frontend handling for NotificationCategory.TargetJvmDiscovery and it includes both FOUND and LOST events.

[hareetd]

TODO once I'm back from PTO:

1. More efficient data handling. Currently, the nested ArchivedRecordingsTable instances for each target in the All Targets table are all populated as soon as one navigates to the Archives page, even though none of the target rows have been expanded yet to show the underlying tables. The same behavior is observed when a target row is expanded or collapsed. This results in a bunch of unnecessary GraphQL queries which can quickly become expensive depending on the number of targets. The proper behavior should be that a nested ArchivedRecordingsTable is only populated when its parent target row is expanded.
2. Search function (search for both targets and recordings?)
3. Better code re-use; there's a lot of overlap between the ArchivedRecordingsTable and the UploadedArchivedRecordingsTable.
4. Unit testing

[hareetd]

@andrewazores

The new Archives page:

Thoughts?

I only have tests left to write. Let me know what you think about my design approach with the implementation itself.

[andrewazores]

Looks and feels good to use. I noticed a bug when I went to "Archives > Uploads". I uploaded an existing recording, but I also had an automated rule running in the background.

My uploaded recording appears in the table, but so do newly archived recordings when notifications come in. Switching tabs to All Archives and back to Uploads does a full re-query and repopulation of the table, so the content goes back to being as expected - until the next archived recording notification comes in.

I think you mentioned something about finding this bug already, but I took a quick look around and didn't see if it's already filed somewhere. The root cause seems to be that the notifications emitted when archives are created/deleted doesn't include information about which archive subdirectory the file was in (which target it originated from). If there is no issue for this bug already then please file one and it can be fixed separately.

Also as an idea for a separate enhancement, it might be nice to give some indication to the user how many archives belong to each target in the Archives > All Targets table. For example adding some graphical element containing a count of recordings for that target. And perhaps if that count is 0 then the table row for that target is not expandable?

[andrewazores]

I also saw this error when trying to delete all archives belonging to a specific target:

The error notification appears and the table does not update, though if I navigate away and back then the archived recordings do appear to be deleted as expected.

[andrewazores]

Selecting each recording file and trying to individually delete it also doesn't result in the table updating. I get the success notification but the archived recording entry is still in the table and selected.

[hareetd]

I think you mentioned something about finding this bug already, but I took a quick look around and didn't see if it's already filed somewhere. The root cause seems to be that the notifications emitted when archives are created/deleted doesn't include information about which archive subdirectory the file was in (which target it originated from). If there is no issue for this bug already then please file one and it can be fixed separately.

I think the issue I filed should cover this case as well, even though the original bug I encountered had to do with ArchivedRecordingDeleted notifications not properly updating state. In both cases, it has to do with the archive subdirectory (i.e. "target") not being properly provided/processed inside the notification.

Also as an idea for a separate enhancement, it might be nice to give some indication to the user how many archives belong to each target in the Archives > All Targets table. For example adding some graphical element containing a count of recordings for that target. And perhaps if that count is 0 then the table row for that target is not expandable?

Good idea, I'll take a look into that.

The error notification appears and the table does not update, though if I navigate away and back then the archived recordings do appear to be deleted as expected.

Selecting each recording file and trying to individually delete it also doesn't result in the table updating. I get the success notification but the archived recording entry is still in the table and selected.

This is another case of the issue I mentioned above. I had originally left this since you marked it as a "good first issue" for Max and Thuan, but if you like I could iron out all the issues we're having with archived recordings (both on the backend and frontend, as we discussed on IRC last week), after this PR.

[andrewazores]

This is another case of the issue I mentioned above. I had originally left this since you marked it as a "good first issue" for Max and Thuan, but if you like I could iron out all the issues we're having with archived recordings (both on the backend and frontend, as we discussed on IRC last week), after this PR.

Makes sense. I'll leave it up to the three of you to decide - @maxcao13 @tthvo any interest in working with Hareet on this bug?

[maxcao13]

This is another case of the issue I mentioned above. I had originally left this since you marked it as a "good first issue" for Max and Thuan, but if you like I could iron out all the issues we're having with archived recordings (both on the backend and frontend, as we discussed on IRC last week), after this PR.

Makes sense. I'll leave it up to the three of you to decide - @maxcao13 @tthvo any interest in working with Hareet on this bug?

Sure, I will take a look at it.

[hareetd]

@maxcao13 Sounds good, let me know if you have any questions.

[hareetd]

I'm a bit stumped on how best to approach keeping a count of the archived recordings per target in the AllTargetsArchivedRecordingsTable (parent table). Ideally, I would like to keep the querying for recordings and subsequent handling encapsulated inside the ArchivedRecordingsTable (child table), allowing the archived recordings data of a target to be queried/loaded only when it's been clicked. However, this means I need a way to trigger an update of the parent table's count state from inside the child table but I'm having trouble with this (see past 5 commits).

Another way would be to add a new GraphQL query that returns a Map consisting of target:count pairs to set the initial state and then use notifications to update the count as archived recordings are added/deleted. Thoughts?

[andrewazores]

Ideally, I would like to keep the querying for recordings and subsequent handling encapsulated inside the ArchivedRecordingsTable (child table), allowing the archived recordings data of a target to be queried/loaded only when it's been clicked. However, this means I need a way to trigger an update of the parent table's count state from inside the child table but I'm having trouble with this (see past 5 commits).

Another way would be to add a new GraphQL query that returns a Map consisting of target:count pairs to set the initial state and then use notifications to update the count as archived recordings are added/deleted. Thoughts?

There are lots of good ideas in here, and I think I agree with the general high-level points overall. But, I think that for the purposes of this feature, the "least evil" is for each sub-table to perform its own query and update only when it's expanded, but also for the overall table to know how to perform essentially the same query but only use the aggregate data (counts). I think this is probably the ideal because it means that on first visit to the view, the main table only ever has to perform one query to populate all N of its rows. If the main table has to rely on the child tables to perform a query, compute the count, and send that data back to the parent, then we need to make N GraphQL queries when visiting this view just for the main table to fully populate.

$ http :8181/api/v2.2/graphql?query="query { targetNodes { name recordings { archived { name } } } }"
HTTP/1.1 200 OK
content-encoding: gzip
content-length: 239
content-type: application/json
vary: origin

{
    "data": {
        "targetNodes": [
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:10000/jmxrmi",
                "recordings": {
                    "archived": []
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9091/jmxrmi",
                "recordings": {
                    "archived": [
                        {
                            "name": "io-cryostat-Cryostat_self_20220610T200712Z.jfr"
                        }
                    ]
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9093/jmxrmi",
                "recordings": {
                    "archived": [
                        {
                            "name": "es-andrewazor-demo-Main_auto_myrule_20220610T200817Z.jfr"
                        },
                        {
                            "name": "es-andrewazor-demo-Main_auto_myrule_20220610T200834Z.jfr"
                        }
                    ]
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9094/jmxrmi",
                "recordings": {
                    "archived": [
                        {
                            "name": "es-andrewazor-demo-Main_auto_myrule_20220610T200817Z.jfr"
                        },
                        {
                            "name": "es-andrewazor-demo-Main_auto_myrule_20220610T200834Z.jfr"
                        }
                    ]
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9095/jmxrmi",
                "recordings": {
                    "archived": [
                        {
                            "name": "es-andrewazor-demo-Main_auto_myrule_20220610T200817Z.jfr"
                        },
                        {
                            "name": "es-andrewazor-demo-Main_auto_myrule_20220610T200833Z.jfr"
                        }
                    ]
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9096/jmxrmi",
                "recordings": {
                    "archived": []
                }
            }
        ]
    }
}

There's still plenty of room to improve this by using a filter on the targetNodes query and implementing pagination, but it's always a single query that can populate all of the high-level information needed by the parent table - it has a name for each target (should correspond 1:1 to what's in the frontend targets.service you're currently referencing for this data) as well as a list of archives belonging to each of those. That nested archived array can just be mapped to its length and displayed into the table Count column on the client side.

My problem with this solution I'm presenting is that it means the client will be receiving a lot of data about archived recordings that it's just going to end up discarding - all we want is the number of recordings in each target archive, but we're getting a whole JSON object with at least the recording's name string in it. This could add up to a lot of wasted bandwidth if each target has a lot of archived recordings. To solve that, here's some reading with ideas:

https://stackoverflow.com/questions/55940123/possible-to-count-result-list-elements-in-type-graphql
https://stackoverflow.com/questions/34321688/can-graphql-return-aggregate-counts

This would suggest that, for example, we could modify our GraphQL Schema for the response type of that recordings structure. Instead of just containing a plain list/array of recording objects, we could restructure that and return a composed object containing the list as well as a "summary" or "aggregates" substructure, which would contain properties like the size of the list.

The query above could then hypothetically look like this instead:

$ http :8181/api/v2.2/graphql?query="query { targetNodes { name recordings { archived { aggregate { count } } } } }"
HTTP/1.1 200 OK
content-encoding: gzip
content-length: 239
content-type: application/json
vary: origin

{
    "data": {
        "targetNodes": [
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:10000/jmxrmi",
                "recordings": {
                    "archived": {
                        "aggregate": {
                            count: 0
                        }
                    }
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9091/jmxrmi",
                "recordings": {
                    "archived": {
                        "aggregate": {
                            count: 1
                        }
                    }
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9093/jmxrmi",
                "recordings": {
                    "archived": {
                        "aggregate": {
                            count: 2
                        }
                    }
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9094/jmxrmi",
                "recordings": {
                    "archived": {
                        "aggregate": {
                            count: 2
                        }
                    }
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9095/jmxrmi",
                "recordings": {
                    "archived": {
                        "aggregate": {
                            count: 2
                        }
                    }
                }
            },
            {
                "name": "service:jmx:rmi:///jndi/rmi://cryostat:9096/jmxrmi",
                "recordings": {
                    "archived": {
                        "aggregate": {
                            count: 0
                        }
                    }
                }
            }
        ]
    }
}

I think updating the parent table in this way is fairly close to an ideal solution. This also means the child tables can continue using the same query and backend implementation as the parent table really - they just use a filter on the top-level query to narrow the view down to their specific target, and they use the actual archived recordings list rather than the aggregate.count property.

[hareetd]

Awesome, appreciate the links/info. That aggregate count approach should work perfectly. Thanks!

[hareetd]

The counts are working now. I still have some implementation work/cleanup to do but I'd appreciate any feedback on how I've designed the AllTargetsArchivedRecordingsTable, particularly with how the Map<Target, number> state representing the targets and their archived recordings counts is maintained and updated.

I orginally tracked the list of targets and their corresponding recordings counts in a Map<Target, number>, storing it in the React state but this resulted in some issues so I separated the Map into two arrays, one for the targets and the other for the counts. I've stress-tested this implementation using Automated Rules and it seems to be functioning well.

However, when I started adding unit tests I realized that when the count in the parent table is updated using a ActiveRecordingSaved or ArchivedRecordingDeleted notification, it results in a new child ArchivedRecordingsTable instance to be created, even though the original ArchivedRecordingsTable instance that was replaced had already added or removed the recording specified by the same notification. I'm trying to figure out the best way to fix this since it defeats the purpose of using notifications to update table state in an efficient manner.

[andrewazores]

However, when I started adding unit tests I realized that when the count in the parent table is updated using a ActiveRecordingSaved or ArchivedRecordingDeleted notification, it results in a new child ArchivedRecordingsTable instance to be created, even though the original ArchivedRecordingsTable instance that was replaced had already added or removed the recording specified by the same notification. I'm trying to figure out the best way to fix this since it defeats the purpose of using notifications to update table state in an efficient manner.

From a quick reading, I think this behaviour is coming from:

https://github.com/cryostatio/cryostat-web/pull/431/files#diff-91d085f18aa7b718a058b31d1d62112c4acbf9dfd73fbf1806c6a6bbd15f5ff1R174

This effect has a dependency on counts, which is updated when either of those two notification types is received (ex. https://github.com/cryostatio/cryostat-web/pull/431/files#diff-91d085f18aa7b718a058b31d1d62112c4acbf9dfd73fbf1806c6a6bbd15f5ff1R199)

So when the notification comes in, the counts is correctly updated, and this cascades into an effect update that at the end calls setSearchedTargets. This (https://github.com/cryostatio/cryostat-web/pull/431/files#diff-91d085f18aa7b718a058b31d1d62112c4acbf9dfd73fbf1806c6a6bbd15f5ff1R288) changes the reference to the targets state that are used for computing the rows, and so the child tables get re-rendered the way you describe.

There might be a way around this by re-ordering your effects carefully, but it seems like that would end up fragile regardless. A little less performant but more robust would be to perform a deep comparison on the targets and counts states whenever you are using them as effect dependencies - just because the array has been replaced doesn't necessarily mean we really need to re-render everything depending on them, because it's possible the new array is conceptually/logically the same as the old array, even though the array reference has changed, as is the case for when the counts have been updated but the targets have not.

https://stackoverflow.com/questions/54095994/react-useeffect-comparing-objects

This looks like an interesting route to explore.

[andrewazores]

Wonderful work, thanks.