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] Expose jvmId through GraphQL query in target metadata #1076

Closed
andrewazores opened this issue Sep 20, 2022 · 5 comments · Fixed by #1089
Closed

[Task] Expose jvmId through GraphQL query in target metadata #1076

andrewazores opened this issue Sep 20, 2022 · 5 comments · Fixed by #1089
Assignees
Labels
bug Something isn't working

Comments

@andrewazores
Copy link
Member

@maxcao13 in doing this I have just noticed that the jvmId comes through in the HTTP REST API metadata, but it isn't available to query via GraphQL. Would you mind taking a look at that?

Originally posted by @andrewazores in https://github.com/cryostatio/cryostat/issues/964#issuecomment-1252439523

@maxcao13 maxcao13 self-assigned this Sep 20, 2022
@andrewazores andrewazores moved this to Todo in 2.2.0 Release Sep 20, 2022
@andrewazores andrewazores added the bug Something isn't working label Sep 20, 2022
@andrewazores andrewazores moved this from Todo to In Progress in 2.2.0 Release Sep 20, 2022
@maxcao13
Copy link
Member

maxcao13 commented Sep 28, 2022

Without looking at the code yet, I'm not sure how the query to http :8181/api/v1/recordings "Authorization: Basic $(echo user:pass | base64)" was able to put the jvmId in the metadata, I don't remember intending that to happen but anyways, I'm leaning towards just removing it from the REST API. What do you think? I'm not sure of the merits of including the jvmId of a TargetNode externally and especially in the discovery database as well since they often change and we only use the id internally anyways.

EDIT: After writing this I remember now that we get the jvmId from the metadata this way StoredRecordingMetadata.getJvmId() and then using that to do all that logic etc... but I can just base 32 decode the subdirectory name and it would work just fine or do some other code magic to remove the jvmId from getting retrieved from an api request. Fixed in corresponding PR.

@andrewazores
Copy link
Member Author

I think it could be useful to include the ID in the API results, actually. Clients may care about specific instances of a target, too. They should only be changing when the JVM restarts, and up to now the only way for a client to know that that has happened has been to be subscribed for WebSocket notifications and use those to update some locally-held state. With the jvmId field present it's now possible for a client to simply periodically poll the REST API or GraphQL API and determine if restarts have happened too, by just comparing the current poll result to the previous one.

Also, I think there's a use case for the cryostat-agent to publish itself with a jvmId as well since it does not necessarily have a meaningful JMX connectUrl, though I haven't yet fleshed out my ideas here exactly. I'm thinking something along the lines of your new beta /:sourceTarget API handlers actually taking the jvmId as the path parameter instead of the JMX URL.

@maxcao13
Copy link
Member

Just to make sure I don't spend wasted time on changes that shouldn't be, does it make sense to add the jvmId to the ServiceRef object which is part of the TargetNode as the main place to query it from? And maybe add the jvmIdHelper to the object class as a lazy dependency which just gets the jvmId from the jvmIdHelper.get(this.serviceUri) whenever the ServiceRef object is constructued?

@andrewazores
Copy link
Member Author

I do think that the ServiceRef is the right place for the ID. I don't think adding a reference to the jvmIdHelper to the ServiceRef is the best thing to do though. Better to put that wherever the ServiceRefs are instantiated and just pass the ID string in as a constructor parameter to the ServiceRef which holds it as an immutable field.

@andrewazores
Copy link
Member Author

The reason I was tinkering around with that ID in the ServiceRef is also related to the comment I made the other day about the new beta API handlers and the /:sourceTarget path param being an ID rather than the JMX service URL. I was experimenting with the -agent generating itself a UUID and including that when it published discovery updates about itself, too. Using the JVM hash ID as the ServiceRef ID for JMX discovery makes a lot of sense, rather than just using a UUID. If I add -core as a dependency to -agent, or if we otherwise make the JVM hash ID function available for use in the -agent, we can also have the agent produce consistent ID results for itself at discovery registration/publish time as what Cryostat would see when connecting to it over JMX.

@maxcao13 maxcao13 moved this from In Progress to Stretch Goals in 2.2.0 Release Oct 17, 2022
Repository owner moved this from Stretch Goals to Done in 2.2.0 Release Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants