add required layer option to conditionally run runtimes queries#1866
Open
khansaad wants to merge 6 commits into
Open
add required layer option to conditionally run runtimes queries#1866khansaad wants to merge 6 commits into
khansaad wants to merge 6 commits into
Conversation
Signed-off-by: Saad Khan <saakhan@ibm.com>
Contributor
Reviewer's GuideImplements conditional execution of runtime-related metric queries by introducing a layers_required attribute on metrics, wiring it into the recommendation engine’s metric filtering, and annotating JVM-related metrics in performance profiles so they only run when corresponding runtime layers are detected. Flow diagram for metric filtering using layers_requiredflowchart TD
A[fetchContainerMetricsBasedOnDataSourceAndProfile] --> B[Call filterMetricsBasedOnExpTypeAndK8sObjectAndLayers]
B --> C[Iterate metric from metricProfile getSloInfo getFunctionVariables]
C --> D{metric.name == maxDateQuery?}
D -->|Yes| SKIP1[Skip metric]
D -->|No| E{ExperimentType matches namespace/container?}
E -->|No| SKIP2[Skip metric]
E -->|Yes| F{Accelerator metric and !fetchAcceleratorMetrics?}
F -->|Yes| SKIP3[Skip metric]
F -->|No| G{layersRequired is null or empty?}
G -->|Yes| H[Include metric]
G -->|No| I{Any layer in layersRequired exists in containerData.layerMap?}
I -->|No| SKIP4[Skip metric<br>and log debug]
I -->|Yes| H[Include metric]
H --> J[Return filtered metricList to fetchContainerMetricsBasedOnDataSourceAndProfile]
SKIP1 --> C
SKIP2 --> C
SKIP3 --> C
SKIP4 --> C
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 left some high level feedback:
- In
fetchContainerMetricsBasedOnDataSourceAndProfile, you repeatedly callcontainerData.getLayerMap()and null-check it inside the inner loop; consider pulling the map into a local variable and short-circuiting when it's null to simplify the logic and avoid repeated lookups. - The
AggregationFunctionsclass now has arequiredLayerfield but no constructor path to set it explicitly; if this type is ever instantiated programmatically (not just via deserialization), you may want to add an overloaded constructor or builder method that acceptsrequiredLayerto avoid partially initialized instances.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `fetchContainerMetricsBasedOnDataSourceAndProfile`, you repeatedly call `containerData.getLayerMap()` and null-check it inside the inner loop; consider pulling the map into a local variable and short-circuiting when it's null to simplify the logic and avoid repeated lookups.
- The `AggregationFunctions` class now has a `requiredLayer` field but no constructor path to set it explicitly; if this type is ever instantiated programmatically (not just via deserialization), you may want to add an overloaded constructor or builder method that accepts `requiredLayer` to avoid partially initialized instances.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>
Contributor
kusumachalasani
left a comment
kusumachalasani
left a comment
There was a problem hiding this comment.
Should we consider updating the profile version ?
Signed-off-by: Saad Khan <saakhan@ibm.com>
12 tasks
Contributor
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
filterMetricsBasedOnExpTypeAndK8sObjectAndLayersmethod largely duplicates the logic offilterMetricsBasedOnExpTypeAndK8sObject; consider refactoring to share the common experiment-type/kubernetes-object filtering to avoid future drift between the two. - The
isROSparameter onfilterMetricsBasedOnExpTypeAndK8sObjectAndLayersis currently unused; either remove it or wire it into the filtering logic to keep the method signature aligned with its behavior. - In the layer-based filtering, you access
containerData.getLayerMap()andcontainerData.getContainer_name(); if there is any chancecontainerDatacan be null in this code path, consider a defensive null check to avoid potential NPEs when adding new call sites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `filterMetricsBasedOnExpTypeAndK8sObjectAndLayers` method largely duplicates the logic of `filterMetricsBasedOnExpTypeAndK8sObject`; consider refactoring to share the common experiment-type/kubernetes-object filtering to avoid future drift between the two.
- The `isROS` parameter on `filterMetricsBasedOnExpTypeAndK8sObjectAndLayers` is currently unused; either remove it or wire it into the filtering logic to keep the method signature aligned with its behavior.
- In the layer-based filtering, you access `containerData.getLayerMap()` and `containerData.getContainer_name()`; if there is any chance `containerData` can be null in this code path, consider a defensive null check to avoid potential NPEs when adding new call sites.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/analyzer/recommendations/engine/RecommendationEngine.java" line_range="1929" />
<code_context>
+ String maxDateQuery,
+ AnalyzerConstants.ExperimentType experimentType,
+ ContainerData containerData,
+ boolean isROS,
+ boolean fetchAcceleratorMetrics,
+ List<String> acceleratorFunctions,
</code_context>
<issue_to_address>
**suggestion:** Remove or use the `isROS` parameter in the new filter method to avoid confusing API surface.
The `filterMetricsBasedOnExpTypeAndK8sObjectAndLayers` method takes an `isROS` parameter but doesn’t use it. Since this is on the recommendation path and likely to be reused, an unused parameter is misleading. Either remove `isROS` if ROS-specific behavior is no longer needed here, or integrate it into the filter logic so the method’s contract matches its signature.
Suggested implementation:
```java
AnalyzerConstants.ExperimentType experimentType,
ContainerData containerData,
boolean fetchAcceleratorMetrics,
List<String> acceleratorFunctions,
List<String> acceleratorPartitionFunctions) {
```
1. Update all call sites of `filterMetricsBasedOnExpTypeAndK8sObjectAndLayers` to remove the `isROS` argument and shift the remaining arguments accordingly.
2. If there is Javadoc or interface declarations elsewhere (e.g., in an interface or superclass) that include the `isROS` parameter for this method, remove that parameter there as well so the signatures stay consistent.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/analyzer/recommendations/engine/RecommendationEngine.java" line_range="1972-1973" />
<code_context>
+if (layersRequired != null && !layersRequired.isEmpty()) {
+ // Check if any of the required layers are detected
+ boolean layerDetected = false;
+ for (String layer : layersRequired) {
+ if (containerData.getLayerMap() != null &&
+ containerData.getLayerMap().containsKey(layer)) {
+ layerDetected = true;
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `containerData` being null when checking `layerMap`, or document the non-null contract.
In `filterMetricsBasedOnExpTypeAndK8sObjectAndLayers`, `containerData.getLayerMap()` is called without first checking whether `containerData` itself is null. If `containerData` can be null at any call site, this will cause an NPE during filtering. If it is guaranteed non-null, consider enforcing that via a precondition at method entry; otherwise, handle a null `containerData` case explicitly (e.g., by short-circuiting the layer filtering).
</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 a new
required_layerattribute in theaggregation_functionsalongside thequeryto conditionally run the runtime queries based on the corresponding layer detection.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
Introduce conditional execution of runtime metrics based on detected layers to reduce unnecessary queries and improve efficiency.
New Features:
Enhancements:
Documentation: