[PR - 0] Add cryostat operator instance and REST API changes#1957
Open
khansaad wants to merge 2 commits into
Open
[PR - 0] Add cryostat operator instance and REST API changes#1957khansaad wants to merge 2 commits into
khansaad wants to merge 2 commits into
Conversation
Signed-off-by: Saad Khan <saakhan@ibm.com>
Contributor
Reviewer's GuideAdds Cryostat as a supported datasource, including a new Cryostat data operator implementation, updates datasource provider selection and supported types, and extends the generic REST client to support POST requests and safer JSON handling needed for Cryostat’s GraphQL API. Sequence diagram for Cryostat POST GraphQL query via GenericRestApiClientsequenceDiagram
participant CryostatDataOperatorImpl
participant GenericRestApiClient
participant HttpClient
CryostatDataOperatorImpl->>GenericRestApiClient: fetchMetricsJson(POST, query)
GenericRestApiClient->>GenericRestApiClient: methodType.equalsIgnoreCase(POST)
alt methodType is POST
GenericRestApiClient->>GenericRestApiClient: create HttpPost(baseURL)
GenericRestApiClient->>GenericRestApiClient: setEntity(StringEntity(query, ContentType.APPLICATION_JSON))
GenericRestApiClient->>GenericRestApiClient: LOGGER.info(Request body)
else methodType is GET
GenericRestApiClient->>GenericRestApiClient: create HttpGet(baseURL + encoded query)
end
GenericRestApiClient->>HttpClient: execute(httpRequestBase)
HttpClient-->>GenericRestApiClient: response
GenericRestApiClient->>GenericRestApiClient: jsonResponse = StringResponseHandler.handleResponse(response)
alt jsonResponse is null or blank
GenericRestApiClient->>GenericRestApiClient: LOGGER.warn(null or empty JSON)
GenericRestApiClient-->>CryostatDataOperatorImpl: new JSONObject()
else jsonResponse not blank
opt methodType is not POST
GenericRestApiClient->>GenericRestApiClient: parse jsonResponse for data.result and warnings
GenericRestApiClient->>GenericRestApiClient: may throw IOException on warnings
end
GenericRestApiClient-->>CryostatDataOperatorImpl: new JSONObject(jsonResponse)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
CryostatDataOperatorImpl.getResultArrayForQuerythe TODO currently results in always returningnull; consider either implementing the GraphQL response parsing now or throwing an explicitUnsupportedOperationExceptionso that callers don’t silently proceed with an unexpectednullresult. - The Cryostat-specific methods (
isServiceable,getValueForQuery,getJsonObjectForQuery) usenullresponses as a generic signal (e.g., for reachability); it may be helpful to distinguish between different failure modes (DNS/host errors vs other IO/parsing errors) so the caller can react more precisely.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `CryostatDataOperatorImpl.getResultArrayForQuery` the TODO currently results in always returning `null`; consider either implementing the GraphQL response parsing now or throwing an explicit `UnsupportedOperationException` so that callers don’t silently proceed with an unexpected `null` result.
- The Cryostat-specific methods (`isServiceable`, `getValueForQuery`, `getJsonObjectForQuery`) use `null` responses as a generic signal (e.g., for reachability); it may be helpful to distinguish between different failure modes (DNS/host errors vs other IO/parsing errors) so the caller can react more precisely.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/common/datasource/DataSourceCollection.java" line_range="109" />
<code_context>
}
- if (!provider.equalsIgnoreCase(KruizeConstants.SupportedDatasources.PROMETHEUS) && !provider.equalsIgnoreCase(KruizeConstants.SupportedDatasources.THANOS)) {
+ if (!KruizeSupportedTypes.DATASOURCES_SUPPORTED.contains(provider)) {
throw new UnsupportedDataSourceProvider(KruizeConstants.DataSourceConstants.DataSourceErrorMsgs.UNSUPPORTED_DATASOURCE_PROVIDER);
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Provider validation is now case-sensitive, which changes existing behavior.
The old logic used `equalsIgnoreCase`, but `DATASOURCES_SUPPORTED` contains only lowercase values and `contains(provider)` is case-sensitive, so inputs like `"PROMETHEUS"` or `"Thanos"` will now fail. Normalize `provider` (e.g., `provider.toLowerCase(Locale.ROOT)`) before checking membership, or ensure the set and inputs are consistently normalized.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/utils/GenericRestApiClient.java" line_range="111-113" />
<code_context>
+ )
+ );
+ httpRequestBase = httpPost;
+ LOGGER.info(
+ "Request body: {}",
+ EntityUtils.toString(
+ ((HttpPost) httpRequestBase).getEntity(),
+ StandardCharsets.UTF_8
</code_context>
<issue_to_address>
**🚨 issue (security):** Logging the full POST request body at INFO may expose sensitive data.
This branch logs the entire JSON body at INFO, which can expose credentials or tokens in logs. Please either lower this to DEBUG, redact sensitive fields, or make it configurable so it can be disabled in production environments.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/autotune/utils/GenericRestApiClient.java" line_range="156-160" />
<code_context>
}
}
}
+ if (jsonResponse == null || jsonResponse.isBlank()) {
+ LOGGER.warn("Received null or empty JSON response");
+ return new JSONObject();
+ }
return new JSONObject(jsonResponse);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning an empty JSONObject on null/blank responses may hide upstream errors.
This change makes an empty or null body indistinguishable from a valid (but empty) JSON object. Callers expecting required fields may now treat an upstream failure as a valid response. Consider signaling this case explicitly (e.g., throw for non-JSON/empty responses or return `null`) so callers can differentiate "no/invalid data" from a successful JSON payload.
```suggestion
if (jsonResponse == null || jsonResponse.isBlank()) {
LOGGER.warn("Received null or empty JSON response");
throw new IOException("Received null or empty JSON response");
}
return new JSONObject(jsonResponse);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Signed-off-by: Saad Khan <saakhan@ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds cryostat datasource client support and REST client handling needed for executing layer queries against cryostat-backed datasources.
Also, Updates datasource abstraction and shared constants/types needed to support multi-datasource layer query execution without including cryostat client-specific implementation changes.
Fixes # (issue)
Type of change
How has this been tested?
Please describe the tests that were run to verify your changes and steps to reproduce. Please specify any test configuration required.
Test Configuration
Checklist 🎯
Additional information
Include any additional information such as links, test results, screenshots here
Summary by Sourcery
Add Cryostat as a supported datasource provider and extend the generic REST client to support POST-based JSON queries and more robust response handling.
New Features:
Enhancements: