[PR-1] Add multi-datasource layer presence detection#1952
Open
khansaad wants to merge 4 commits into
Open
Conversation
Contributor
Reviewer's GuideImplements multi-datasource-aware layer presence detection, including a special Cryostat+Prometheus flow, by generalizing detectors to accept multiple datasource names and adding shared utilities for pod extraction and datasource constants. Sequence diagram for Cryostat multi-datasource layer presence detectionsequenceDiagram
participant LayerUtils as LayerUtils
participant QueryDetector as QueryBasedPresence
participant DSCollection as DataSourceCollection
participant DSOperator as DataSourceOperatorImpl
participant PromOp as DataSourceOperatorImpl_prometheus
participant CryostatOp as DataSourceOperatorImpl_cryostat
LayerUtils->>QueryDetector: detectPresence(namespace, containerName, datasourceNames)
QueryDetector->>QueryDetector: iterate queries
QueryDetector->>QueryDetector: iterate datasourceNames
alt query.getDataSource == CRYOSTAT
QueryDetector->>DSCollection: getInstance().getDataSourcesCollection()
QueryDetector->>QueryDetector: find promDatasourceInfo
alt promDatasourceInfo found
QueryDetector->>PromOp: getJsonObjectForQuery(promDatasourceInfo, promQl)
PromOp-->>QueryDetector: JSONObject
QueryDetector->>LayerUtils: extractPods(promResponse)
LayerUtils-->>QueryDetector: List<String> pods
QueryDetector->>CryostatOp: getOperator(CRYOSTAT)
loop pods
QueryDetector->>CryostatOp: getJsonObjectForQuery(dataSourceInfo, queryToTry)
CryostatOp-->>QueryDetector: JSONObject graphQlJson
alt envNodes not empty
QueryDetector-->>LayerUtils: return true
end
end
else promDatasourceInfo not found
QueryDetector-->>QueryDetector: skip Cryostat detection
end
else other datasource
QueryDetector->>DSCollection: getInstance().getDataSourcesCollection()
QueryDetector->>DSOperator: getOperator(provider)
QueryDetector->>DSOperator: getResultArrayForQuery(dataSourceInfo, modifiedQuery)
DSOperator-->>QueryDetector: JsonArray
alt resultArray not empty
QueryDetector-->>LayerUtils: return true
end
end
QueryDetector-->>LayerUtils: return false
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
QueryBasedPresence.detectPresence, theif (query.getDataSource().equalsIgnoreCase(KruizeConstants.SupportedDatasources.CRYOSTAT))branch can trigger an NPE whengetDataSource()is null; consider reusing the earlier null-check pattern (or normalizing to a non-null string) before callingequalsIgnoreCase. - In
LayerUtils.extractPods,promResponseis dereferenced without a null check; adding a simpleif (promResponse == null) return pods;at the top would avoid potential NPEs when the datasource call fails or returns null. - The Cryostat detection path recomputes the Prometheus datasource (
promDatasourceInfo) inside nested loops for each datasource/query combination; consider resolving the Prometheus datasource once perdetectPresenceinvocation and reusing it to reduce repeated lookups and simplify the control flow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `QueryBasedPresence.detectPresence`, the `if (query.getDataSource().equalsIgnoreCase(KruizeConstants.SupportedDatasources.CRYOSTAT))` branch can trigger an NPE when `getDataSource()` is null; consider reusing the earlier null-check pattern (or normalizing to a non-null string) before calling `equalsIgnoreCase`.
- In `LayerUtils.extractPods`, `promResponse` is dereferenced without a null check; adding a simple `if (promResponse == null) return pods;` at the top would avoid potential NPEs when the datasource call fails or returns null.
- The Cryostat detection path recomputes the Prometheus datasource (`promDatasourceInfo`) inside nested loops for each datasource/query combination; consider resolving the Prometheus datasource once per `detectPresence` invocation and reusing it to reduce repeated lookups and simplify the control flow.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java" line_range="117-126" />
<code_context>
+ if (query.getDataSource().equalsIgnoreCase(KruizeConstants.SupportedDatasources.CRYOSTAT)) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against null dataSource when checking for Cryostat to avoid NPE
This line assumes `query.getDataSource()` is non-null, but earlier you treat it as nullable, so this can throw an NPE. Please either combine this with the earlier null check (e.g. `if (query.getDataSource() != null && query.getDataSource().equalsIgnoreCase(CRYOSTAT))`) or reverse the equals: `if (KruizeConstants.SupportedDatasources.CRYOSTAT.equalsIgnoreCase(query.getDataSource())) { ... }` to avoid NPEs.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java" line_range="139-148" />
<code_context>
+ DataSourceOperatorImpl prometheusOperator = DataSourceOperatorImpl.getInstance()
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle potential null operators for Prometheus and Cryostat to avoid runtime NPEs
Elsewhere you check for `operator == null` and log/continue, but here `prometheusOperator` and `cryostatOperator` are used without guards. If the lookup fails (e.g., misconfiguration or unsupported provider), `getJsonObjectForQuery` can throw an NPE. Please apply the same guard pattern here (log a warning and `continue`) so failures are handled gracefully instead of crashing.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/autotune/analyzer/kruizeLayer/utils/LayerUtils.java" line_range="132-137" />
<code_context>
return detectedLayers;
}
+
+ public static List<String> extractPods(JSONObject promResponse) {
+ List<String> pods = new ArrayList<>();
+
+ JSONObject data = promResponse.optJSONObject("data");
+ if (data == null) {
+ return pods;
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Add null-safety for promResponse and intermediate JSON objects in extractPods
`promResponse` can be null, which would NPE on `promResponse.optJSONObject("data")`. Similarly, `results.optJSONObject(i)` may return null, leading to an NPE when accessing `"metric"`. To keep this utility robust, early-return if `promResponse` is null and guard each per-result access:
```java
if (promResponse == null) {
return pods;
}
...
JSONObject result = results.optJSONObject(i);
if (result == null) {
continue;
}
JSONObject metric = result.optJSONObject("metric");
...
```
</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>
Signed-off-by: Saad Khan <saakhan@ibm.com>
Signed-off-by: Saad Khan <saakhan@ibm.com>
Signed-off-by: Saad Khan <saakhan@ibm.com>
khansaad
force-pushed
the
mds-pr-1-presence-core
branch
from
db43529 to
ffc0c00
Compare
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
Adds the core layer presence detection flow for multi-datasource support, including query-based, label-based, and always-present strategies along with shared helper logic.
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
Support detecting Kruize layer presence using multiple datasources and add Cryostat-specific detection flow on top of the existing query-based mechanism.
New Features:
Enhancements: