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(graphql): add aggregate information for active recordings in the GraphQL API #1066

Merged
merged 7 commits into from
Sep 26, 2022

Conversation

maxcao13
Copy link
Member

@maxcao13 maxcao13 commented Sep 14, 2022

Fixes #1030

@maxcao13 maxcao13 added the feat New feature or request label Sep 14, 2022
@andrewazores
Copy link
Member

I think this will depend on a -web PR to be created as well: cryostatio/cryostat-web#473 (comment)

@andrewazores
Copy link
Member

Related: #1077

@maxcao13 can you resume work on this PR so we can be sure it gets into this release alongside the linked one above? Otherwise the API structure becomes asymmetric between active and archived.

@maxcao13
Copy link
Member Author

@maxcao13 can you resume work on this PR so we can be sure it gets into this release alongside the linked one above? Otherwise the API structure becomes asymmetric between active and archived.

Sure, no problem.

@maxcao13
Copy link
Member Author

The PR doesn't included the new 'size' aggregate info, do you want me to include it here?

@maxcao13 maxcao13 marked this pull request as ready for review September 23, 2022 18:29
@maxcao13
Copy link
Member Author

Also, a query like this:

$ http :8181/api/v2.2/graphql Authorization:"Basic $(echo user:pass | base64)" query="query { targetNodes { name recordings { active { aggregate { count } } }
 } }"

results in

{
    "data": null,
    "errors": [
        {
            "extensions": {
                "classification": "DataFetchingException"
            },
            "locations": [
                {
                    "column": 28,
                    "line": 1
                }
            ],
            "message": "Exception while fetching data (/targetNodes[4]/recordings) : Failed to retrieve RMIServer stub: javax.naming.CommunicationException [Root exception is java.rmi.ConnectIOException: non-JRMP server at remote endpoint]",
            "path": [
                "targetNodes",
                4,
                "recordings"
            ]
        }
    ]
}

if a target is unreachable (in my case 9095 with no SSL cert), but this is fixed in the archive endpoint PR #1047, by the try catch in the ActiveRecordingFetcher.

This is a query on reachable targets using smoketest (for me who doesn't know how to validate the SSL for 9095).

$ http :8181/api/v2.2/graphql Authorization:"Basic $(echo user:pass | base64)" query="query { environmentNodes(filter: { name: \"quarkus-test\" }) { name nodeType descendantTargets { recordings { active { data { name state metadata { labels } } } archived { data { name metadata { labels } } } } } } }"

@andrewazores
Copy link
Member

The PR doesn't included the new 'size' aggregate info, do you want me to include it here?

I'm not sure if we can easily do that right now - the JDK Recording class has an accessor for this, but the JMC IRecordingDescriptor does not. The information is available over JMX with an accessor like the Recording one, but to use this we would need to update -core. This could be something to propose for JMC to add upstream, too.

In theory this would be a nice addition but in practice I don't think we should go for it, not right now.

@andrewazores
Copy link
Member

andrewazores commented Sep 24, 2022

(for me who doesn't know how to validate the SSL for 9095).

cryostatio/cryostatio.github.io#70

+

https://github.com/andrewazores/vertx-fib-demo/blob/master/src/main/extras/app/resources/vertx-fib-demo.cer

andrewazores
andrewazores previously approved these changes Sep 26, 2022
@andrewazores andrewazores merged commit 391a9d8 into cryostatio:main Sep 26, 2022
@maxcao13 maxcao13 deleted the aggregate-active branch September 26, 2022 22:12
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
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Add aggregate information for active recordings in the GraphQL API
2 participants