Layers - 07 Layer Detection for Query based layer presence #1770
+354
−1
Conversation
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
Contributor
Reviewer's GuideIntroduce a unified runtime layer presence detection path, including a query-based detector that augments PromQL with workload context, an always-present detector implementation, and a LayerUtils helper that loads layers from the DB and resolves which layers are active for a given container/namespace/workload, along with minor robustness improvements to DB conversion helpers. Sequence diagram for query-based layer detection via LayerUtilssequenceDiagram
actor Caller
participant LayerUtils
participant ExperimentDBService
participant DB as Database
participant KruizeLayer
participant LayerPresence
participant Detector as LayerPresenceDetector
participant QueryDetector as QueryBasedPresence
participant DSCollection as DataSourceCollection
participant DSOpImpl as DataSourceOperatorImpl
participant DataSourceInfo
participant MetricStore
Caller->>LayerUtils: detectLayers(containerName, workloadName, namespace)
alt invalid containerName or namespace
LayerUtils-->>Caller: null
else valid inputs
LayerUtils->>ExperimentDBService: loadAllLayers(allLayersMap)
ExperimentDBService->>DB: query all layers
DB-->>ExperimentDBService: layer records
ExperimentDBService-->>LayerUtils: allLayersMap populated
loop for each KruizeLayer in allLayersMap
LayerUtils->>KruizeLayer: getLayerPresence()
KruizeLayer-->>LayerUtils: LayerPresence
LayerUtils->>LayerPresence: getDetector()
LayerPresence-->>LayerUtils: detector
alt detector is QueryBasedPresence
LayerUtils->>QueryDetector: detectPresence(namespace, workloadName, containerName)
QueryDetector->>DSCollection: getInstance()
DSCollection-->>QueryDetector: DataSourceCollection
QueryDetector->>DSCollection: getDataSourcesCollection()
DSCollection-->>QueryDetector: Map
QueryDetector->>DSCollection: get(query.dataSource)
DSCollection-->>QueryDetector: DataSourceInfo
alt DataSourceInfo found
QueryDetector->>DSOpImpl: getInstance()
DSOpImpl-->>QueryDetector: DataSourceOperatorImpl
QueryDetector->>DSOpImpl: getOperator(query.dataSource)
DSOpImpl-->>QueryDetector: operator
alt operator found
QueryDetector->>QueryDetector: appendFilter(query, namespace)
QueryDetector->>QueryDetector: appendFilter(query, workloadName)
QueryDetector->>QueryDetector: appendFilter(query, containerName)
QueryDetector->>DSOpImpl: getResultArrayForQuery(DataSourceInfo, modifiedQuery)
DSOpImpl->>MetricStore: execute PromQL
MetricStore-->>DSOpImpl: JsonArray results
DSOpImpl-->>QueryDetector: JsonArray
alt results not empty
QueryDetector-->>LayerUtils: true
LayerUtils->>LayerUtils: add layer to detectedLayers
else results empty
QueryDetector-->>LayerUtils: false
end
else operator missing
QueryDetector-->>LayerUtils: false
end
else DataSourceInfo missing
QueryDetector-->>LayerUtils: false
end
else other detector type
LayerUtils->>Detector: detectPresence(namespace, workloadName)
Detector-->>LayerUtils: boolean
alt true
LayerUtils->>LayerUtils: add layer to detectedLayers
end
end
end
alt detectedLayers empty
LayerUtils-->>Caller: null
else some layers detected
LayerUtils-->>Caller: detectedLayers
end
end
Updated class diagram for layer presence detection componentsclassDiagram
class LayerPresenceDetector {
<<interface>>
+PresenceType getType()
+boolean detectPresence(String namespace, String workloadName)
}
class PresenceAlways {
+String presence
+PresenceAlways()
+PresenceType getType()
+boolean detectPresence(String namespace, String workloadName)
+String getPresence()
+void setPresence(String presence)
+String toString()
}
class QueryBasedPresence {
-Logger LOGGER
-List~LayerPresenceQuery~ queries
+PresenceType getType()
+boolean detectPresence(String namespace, String workloadName)
+boolean detectPresence(String namespace, String workloadName, String containerName)
-String appendFilter(String query, String labelName, String labelValue)
+List~LayerPresenceQuery~ getQueries()
}
class LabelBasedPresence {
-List~LayerPresenceLabel~ label
+PresenceType getType()
+boolean detectPresence(String namespace, String workloadName)
+List~LayerPresenceLabel~ getLabel()
+void setLabel(List~LayerPresenceLabel~ label)
}
class KruizeLayer {
+String apiVersion
+String kind
+String layerName
+LayerPresence layerPresence
+String getApiVersion()
+String getKind()
+String getLayerName()
+LayerPresence getLayerPresence()
}
class LayerPresence {
+LayerPresenceDetector detector
+PresenceType type
+LayerPresenceDetector getDetector()
+PresenceType getType()
}
class LayerUtils {
+static Map~String, KruizeLayer~ detectLayers(String containerName, String workloadName, String namespace)
}
class ExperimentDBService {
+void loadAllLayers(Map~String, KruizeLayer~ allLayersMap) throws Exception
}
class DataSourceCollection {
+static DataSourceCollection getInstance()
+Map~String, DataSourceInfo~ getDataSourcesCollection()
}
class DataSourceInfo {
}
class DataSourceOperatorImpl {
+static DataSourceOperatorImpl getInstance()
+DataSourceOperatorImpl getOperator(String dataSourceName)
+JsonArray getResultArrayForQuery(DataSourceInfo dataSourceInfo, String query) throws Exception
}
class LayerPresenceQuery {
+String getDataSource()
+String getLayerPresenceQuery()
}
class LayerPresenceLabel {
}
LayerPresenceDetector <|.. PresenceAlways
LayerPresenceDetector <|.. QueryBasedPresence
LayerPresenceDetector <|.. LabelBasedPresence
KruizeLayer --> LayerPresence : layerPresence
LayerPresence --> LayerPresenceDetector : detector
LayerUtils --> KruizeLayer : detects
LayerUtils --> ExperimentDBService : uses
QueryBasedPresence --> LayerPresenceQuery : queries
QueryBasedPresence --> DataSourceCollection : resolvesDataSource
QueryBasedPresence --> DataSourceOperatorImpl : executesQuery
DataSourceCollection --> DataSourceInfo : returns
LabelBasedPresence --> LayerPresenceLabel : label
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 6 issues, and left some high level feedback:
- There is duplicated KruizeLayer↔KruizeLMLayerEntry conversion logic (DBHelpers.Converters.convertLayerObjectToLayerDBObj / convertLayerDBObjToLayerObject and LayerService.convertToLayerEntry) using both Gson and Jackson; consider consolidating into a single helper and standardising on one JSON library to avoid divergence and subtle serialization differences.
- Several new methods catch broad Exception, log only the message, and sometimes call e.printStackTrace() (e.g. DBHelpers layer converters, ExperimentDAOImpl.addLayerToDB/loadAllLayers/loadLayerByName, LayerService.doPost/doGet); tightening the exception types and relying on structured logging instead of printStackTrace() will make failures easier to track and avoid masking errors.
- Layer presence types are defined both in AnalyzerConstants.LayerConstants.PresenceType and in LayerPresenceDetector.PresenceType, and LabelBasedPresence.detectPresence currently throws UnsupportedOperationException; consider unifying the enum source and either guarding against or providing a safe no-op implementation for label-based presence to avoid unexpected runtime failures when a LABEL presence is configured.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is duplicated KruizeLayer↔KruizeLMLayerEntry conversion logic (DBHelpers.Converters.convertLayerObjectToLayerDBObj / convertLayerDBObjToLayerObject and LayerService.convertToLayerEntry) using both Gson and Jackson; consider consolidating into a single helper and standardising on one JSON library to avoid divergence and subtle serialization differences.
- Several new methods catch broad Exception, log only the message, and sometimes call e.printStackTrace() (e.g. DBHelpers layer converters, ExperimentDAOImpl.addLayerToDB/loadAllLayers/loadLayerByName, LayerService.doPost/doGet); tightening the exception types and relying on structured logging instead of printStackTrace() will make failures easier to track and avoid masking errors.
- Layer presence types are defined both in AnalyzerConstants.LayerConstants.PresenceType and in LayerPresenceDetector.PresenceType, and LabelBasedPresence.detectPresence currently throws UnsupportedOperationException; consider unifying the enum source and either guarding against or providing a safe no-op implementation for label-based presence to avoid unexpected runtime failures when a LABEL presence is configured.
## Individual Comments
### Comment 1
<location> `src/main/java/com/autotune/database/helper/DBHelpers.java:1570-1560` </location>
<code_context>
+ public static KruizeLMLayerEntry convertLayerObjectToLayerDBObj(com.autotune.analyzer.kruizeLayer.KruizeLayer kruizeLayer) {
</code_context>
<issue_to_address>
**suggestion:** Simplify exception handling and avoid `e.printStackTrace()` in layer-to-DB conversion
The method currently wraps `JsonProcessingException` in a generic `Exception`, then immediately catches `Exception` and logs only `e.getMessage()` plus `e.printStackTrace()`. This obscures the original exception type in logs and duplicates stack trace output to stdout/stderr.
Instead, log the original throwable directly (e.g. `LOGGER.error("...", e)`) and avoid wrapping in a generic `Exception`—either let `JsonProcessingException` propagate to the outer catch or handle it inline with proper context. If this path is performance-sensitive, also consider reusing a shared `ObjectMapper`/`Gson` instance instead of creating a new one each time.
</issue_to_address>
### Comment 2
<location> `src/main/java/com/autotune/database/helper/DBHelpers.java:1624-1633` </location>
<code_context>
+ public static KruizeLayer convertLayerDBObjToLayerObject(KruizeLMLayerEntry entry) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid generic `Exception` wrapping and `e.printStackTrace()` when converting DB layer entry to domain object
This method mirrors the other converter by catching generic `Exception`, logging only `e.getMessage()`, calling `printStackTrace()`, and returning `null`, which hides failures and drops bad records.
Please:
- Log the full exception (e.g. `LOGGER.error("Failed to convert KruizeLMLayerEntry", e)`) instead of using `printStackTrace()`.
- Avoid wrapping everything in generic `Exception`; either let specific exceptions propagate or handle/log them where they occur.
- Revisit returning `null` on failure; if conversion must be reliable, prefer propagating the exception so callers can handle partial failures explicitly.
</issue_to_address>
### Comment 3
<location> `src/main/java/com/autotune/database/helper/DBHelpers.java:1683-1692` </location>
<code_context>
+ public static List<KruizeLayer> convertLayerEntryToLayerObject(List<KruizeLMLayerEntry> kruizeLayerEntryList) throws Exception {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify or simplify exception contract in `convertLayerEntryToLayerObject`
The method declares `throws Exception` and wraps the whole loop in a `try/catch`, but you already catch and log exceptions from `convertLayerDBObjToLayerObject` inside the loop. This means the outer `catch` will rarely trigger and the exception contract is misleading.
Please either:
- Remove the outer `try/catch` and `throws Exception` if individual failures should be skipped, or
- Let `convertLayerDBObjToLayerObject` throw and handle the failure here if the entire conversion should fail on error.
Clarifying this will make the behavior predictable to callers.
Suggested implementation:
```java
* Converts List of KruizeLMLayerEntry DB objects to List of KruizeLayer objects
*
* @param kruizeLayerEntryList List of KruizeLMLayerEntry database objects to be converted
* @return List of KruizeLayer domain objects
*/
public static List<KruizeLayer> convertLayerEntryToLayerObject(List<KruizeLMLayerEntry> kruizeLayerEntryList) {
List<KruizeLayer> kruizeLayerList = new ArrayList<>();
for (KruizeLMLayerEntry entry : kruizeLayerEntryList) {
try {
KruizeLayer kruizeLayer = convertLayerDBObjToLayerObject(entry);
if (kruizeLayer != null) {
kruizeLayerList.add(kruizeLayer);
}
} catch (Exception e) {
LOGGER.error("Error occurred while converting layer entry to layer object: {}", e.getMessage());
```
Because the method signature no longer declares `throws Exception`, any call sites that currently rely on catching or rethrowing that checked exception can be simplified. Specifically:
1. Remove any `try/catch` blocks whose sole purpose is to handle `Exception` thrown by `convertLayerEntryToLayerObject`.
2. Update any method signatures that declared `throws Exception` only because they called `convertLayerEntryToLayerObject`.
</issue_to_address>
### Comment 4
<location> `src/main/java/com/autotune/analyzer/services/LayerService.java:69-78` </location>
<code_context>
+ KruizeLayer kruizeLayer = Converters.KruizeObjectConverters.convertInputJSONToCreateLayer(inputData);
</code_context>
<issue_to_address>
**suggestion:** Reuse the existing DB helper converter instead of duplicating mapping logic in `LayerService`
`DBHelpers.Converters.KruizeObjectConverters.convertLayerObjectToLayerDBObj` already maps `KruizeLayer` to `KruizeLMLayerEntry`. `LayerService`’s `convertToLayerEntry` duplicates this logic. Prefer reusing the existing converter (or delegating to it) unless this API truly requires a different mapping, to keep behavior consistent and reduce maintenance.
Suggested implementation:
```java
private KruizeLMLayerEntry convertToLayerEntry(KruizeLayer kruizeLayer) {
// Delegate to the shared DB helper converter to keep mapping logic centralized
return Converters.KruizeObjectConverters.convertLayerObjectToLayerDBObj(kruizeLayer);
}
```
If the body of `convertToLayerEntry` in your codebase differs from the one shown in the SEARCH block, adjust the SEARCH section to match your actual implementation and keep the REPLACE section as-is. The key change is that `convertToLayerEntry` should no longer manually construct and populate `KruizeLMLayerEntry`; it should simply call `Converters.KruizeObjectConverters.convertLayerObjectToLayerDBObj(kruizeLayer)`.
</issue_to_address>
### Comment 5
<location> `src/main/java/com/autotune/analyzer/services/LayerService.java:159-161` </location>
<code_context>
+ );
+ }
+
+ if (!error) {
+ Gson gsonObj = createGsonObject();
+ String gsonStr = gsonObj.toJson(layerEntries);
+ response.getWriter().println(gsonStr);
+ response.getWriter().close();
</code_context>
<issue_to_address>
**suggestion:** Consider returning domain `KruizeLayer` objects instead of DB entities from list API
This endpoint currently serializes `KruizeLMLayerEntry` (the persistence entity) directly, even though the rest of the codebase uses `KruizeLayer` as the domain model and already has converters. This couples the API to the DB schema. Instead, convert `layerEntries` to `KruizeLayer` and serialize those, keeping the DB entity internal to the persistence layer.
Suggested implementation:
```java
if (!error) {
// Convert persistence entities to domain model before serializing
List<KruizeLayer> kruizeLayers = layerEntries.stream()
.map(layerEntry -> KruizeLayerConverter.toKruizeLayer(layerEntry))
.collect(Collectors.toList());
Gson gsonObj = createGsonObject();
String gsonStr = gsonObj.toJson(kruizeLayers);
response.getWriter().println(gsonStr);
response.getWriter().close();
}
```
1. Ensure that a converter from the persistence entity to the domain model exists and is imported:
- Import the domain model and converter at the top of `LayerService.java`, for example:
- `import com.autotune.analyzer.models.KruizeLayer;`
- `import com.autotune.analyzer.converters.KruizeLayerConverter;`
- `import java.util.stream.Collectors;`
- Adjust `KruizeLayerConverter.toKruizeLayer(...)` to match the actual converter utility and method name in your codebase (for example it might be `KruizeLayerConvertor`, `LayerMapper`, `DBLayerToKruizeLayerConverter`, or a static `from(...)` method on `KruizeLayer`).
2. If the converter operates on collections instead of single entities, replace the `stream().map(...).collect(...)` with a direct call such as `KruizeLayerConverter.toKruizeLayers(layerEntries)`.
3. If generics or type inference cause compilation issues, explicitly declare the type arguments in the stream pipeline or adjust imports to avoid naming collisions.
</issue_to_address>
### Comment 6
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/presence/LabelBasedPresence.java:39-47` </location>
<code_context>
+ this.tunables = tunables;
+ }
+
+ @Override
+ public String toString() {
+ return "KruizeLayer{" +
</code_context>
<issue_to_address>
**issue (bug_risk):** Unimplemented label-based presence detection will throw at runtime for LABEL presence types
For `PresenceType.LABEL`, `detectPresence` currently throws `UnsupportedOperationException`, so any layer configured with label-based presence will fail at runtime when `LayerPresence.getDetector().detectPresence(...)` is called.
If LABEL presence isn’t supported yet, either reject such configurations earlier (e.g., validation that disallows `PresenceType.LABEL`) or temporarily return `false` with a warning log instead of throwing. This will prevent unexpected 500s if a LABEL configuration is accidentally used.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
12 tasks
bhanvimenghani
force-pushed
the
layer-detection
branch
from
f96817f to
d2153d1
Compare
Contributor
|
@bhanvimenghani Please fix the conflicts |
bhanvimenghani
force-pushed
the
layer-detection
branch
from
d2153d1 to
3a929bb
Compare
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
DBHelpers.convertLayerObjectToLayerDBObj, the method no longer validateskruizeLayeror throws on failure and can now quietly returnnullafter catching exceptions; consider reintroducing explicit validation and/or propagating or logging errors so callers don’t silently proceed with an invalid DB object. LayerUtils.detectLayersreturnsnullin multiple situations (invalid input, DB load failure, no layers in DB, no layers detected), which makes it hard for callers to distinguish between error and “no layers detected”; consider returning an empty map for the latter case(s) and reservingnullfor genuine failures.- The
presencefield added toPresenceAlwaysis never used in detection and only exposed via getters/setters; either wire it into the behavior or remove it to avoid confusion and dead state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DBHelpers.convertLayerObjectToLayerDBObj`, the method no longer validates `kruizeLayer` or throws on failure and can now quietly return `null` after catching exceptions; consider reintroducing explicit validation and/or propagating or logging errors so callers don’t silently proceed with an invalid DB object.
- `LayerUtils.detectLayers` returns `null` in multiple situations (invalid input, DB load failure, no layers in DB, no layers detected), which makes it hard for callers to distinguish between error and “no layers detected”; consider returning an empty map for the latter case(s) and reserving `null` for genuine failures.
- The `presence` field added to `PresenceAlways` is never used in detection and only exposed via getters/setters; either wire it into the behavior or remove it to avoid confusion and dead state.
## Individual Comments
### Comment 1
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java:57-65` </location>
<code_context>
+ public boolean detectPresence(String namespace, String workloadName, String containerName) throws Exception {
</code_context>
<issue_to_address>
**suggestion:** The `workloadName` parameter is accepted but never used in query construction.
If `workloadName` is intended to affect the query, it should be wired into `modifiedQuery` (or the filter helper) alongside `namespace` and `containerName`. If not, please remove it from the method signature, or add a brief TODO indicating the planned usage to avoid confusion.
```suggestion
/**
* Detect layer presence with optional container name filtering
* @param namespace The Kubernetes namespace
* @param workloadName The workload name
* @param containerName The container name (optional)
* @return true if layer is detected
* @throws Exception if detection fails
*
* TODO: Wire workloadName into query construction (or remove the parameter)
* once workload-level filtering requirements are finalized.
*/
public boolean detectPresence(String namespace, String workloadName, String containerName) throws Exception {
// TODO: Include workloadName in query construction when workload-level filtering is implemented.
```
</issue_to_address>
### Comment 2
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java:118-122` </location>
<code_context>
+ namespace, containerName);
+ return true;
+ }
+ } catch (Exception e) {
+ LOGGER.error("Error executing layer presence query for datasource '{}': {}",
+ query.getDataSource(), e.getMessage());
+ // Continue to next query instead of failing completely
</code_context>
<issue_to_address>
**suggestion:** The error log on query failure loses stack trace information.
Only logging `e.getMessage()` drops the stack trace and makes production debugging harder. Please log the exception as a throwable parameter instead, e.g. `LOGGER.error("Error executing layer presence query for datasource '{}': {}", query.getDataSource(), e.getMessage(), e);` or `LOGGER.error("Error executing layer presence query for datasource '{}'", query.getDataSource(), e);`.
```suggestion
} catch (Exception e) {
LOGGER.error("Error executing layer presence query for datasource '{}': {}",
query.getDataSource(), e.getMessage(), e);
// Continue to next query instead of failing completely
}
```
</issue_to_address>
### Comment 3
<location> `src/main/java/com/autotune/database/helper/DBHelpers.java:1574-1577` </location>
<code_context>
* @param kruizeLayer KruizeLayer object to be converted
* @return KruizeLMLayerEntry database entry object
*/
- public static KruizeLMLayerEntry convertLayerObjectToLayerDBObj(KruizeLayer kruizeLayer) throws Exception {
- if (kruizeLayer == null) {
- throw new IllegalArgumentException("KruizeLayer cannot be null");
- }
-
+ public static KruizeLMLayerEntry convertLayerObjectToLayerDBObj(com.autotune.analyzer.kruizeLayer.KruizeLayer kruizeLayer) {
+ KruizeLMLayerEntry kruizeLayerEntry = null;
try {
- KruizeLMLayerEntry kruizeLayerEntry = new KruizeLMLayerEntry();
+ kruizeLayerEntry = new KruizeLMLayerEntry();
kruizeLayerEntry.setApi_version(kruizeLayer.getApiVersion());
kruizeLayerEntry.setKind(kruizeLayer.getKind());
</code_context>
<issue_to_address>
**issue (bug_risk):** Behavior change: null input no longer validated and errors now result in null return instead of exceptions.
The previous API guaranteed a non-null `kruizeLayer` and surfaced failures via exceptions; the new behavior both accepts `null` and swallows any thrown exception, returning `null` instead. This can introduce subtle bugs for callers that rely on exceptions or assume a non-null result. Consider restoring the explicit null check and propagating (or wrapping) exceptions after logging, or, if the new contract is intentional, clearly document it and update all callers to handle a `null` return.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java
Show resolved
Hide resolved
Comment on lines
1574
to
1577
| public static KruizeLMLayerEntry convertLayerObjectToLayerDBObj(KruizeLayer kruizeLayer) throws Exception { | ||
| if (kruizeLayer == null) { | ||
| throw new IllegalArgumentException("KruizeLayer cannot be null"); | ||
| } |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Behavior change: null input no longer validated and errors now result in null return instead of exceptions.
The previous API guaranteed a non-null kruizeLayer and surfaced failures via exceptions; the new behavior both accepts null and swallows any thrown exception, returning null instead. This can introduce subtle bugs for callers that rely on exceptions or assume a non-null result. Consider restoring the explicit null check and propagating (or wrapping) exceptions after logging, or, if the new contract is intentional, clearly document it and update all callers to handle a null return.
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
QueryBasedPresence.appendFilter, label values are interpolated directly into the PromQL without any escaping; consider properly escaping quotes and other special characters inlabelValueto avoid generating invalid or exploitable queries. - In
LayerUtils.detectLayers, returningnullfor many failure/empty cases (invalid input, DB load failure, no layers, no detections) makes it harder for callers to distinguish states; consider consistently returning an empty map and using log messages or a result wrapper to communicate why no layers were detected.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `QueryBasedPresence.appendFilter`, label values are interpolated directly into the PromQL without any escaping; consider properly escaping quotes and other special characters in `labelValue` to avoid generating invalid or exploitable queries.
- In `LayerUtils.detectLayers`, returning `null` for many failure/empty cases (invalid input, DB load failure, no layers, no detections) makes it harder for callers to distinguish states; consider consistently returning an empty map and using log messages or a result wrapper to communicate why no layers were detected.
## Individual Comments
### Comment 1
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java:103-104` </location>
<code_context>
+ if (workloadName != null && !workloadName.isBlank()) {
+ modifiedQuery = appendFilter(modifiedQuery, "pod", workloadName);
+ }
+ if (containerName != null && !containerName.isBlank()) {
+ modifiedQuery = appendFilter(modifiedQuery, "container", containerName);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Container name is directly interpolated into the query without escaping, which can break the PromQL if special characters are present.
Even though container names are often simple, they can still include characters that break PromQL (e.g., quotes). Please sanitize or escape `namespace`, `workloadName`, and `containerName` before building the query, or handle label escaping within `appendFilter` so queries are always valid.
</issue_to_address>
### Comment 2
<location> `src/main/java/com/autotune/analyzer/kruizeLayer/utils/LayerUtils.java:47-49` </location>
<code_context>
+ String workloadName,
+ String namespace) {
+ // Validate inputs
+ if (containerName == null || containerName.isBlank()) {
+ LOGGER.warn("Container name is null or empty, cannot detect layers");
+ return null;
+ }
+ if (namespace == null || namespace.isBlank()) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning null for invalid inputs conflates "no layers detected" with "inputs invalid", complicating caller logic.
This method returns `null` for multiple cases (invalid input, DB failures, no layers in DB, and no layers detected), while a non-null empty map is also possible. That makes it difficult for callers to distinguish "no layers present" from "detection/lookup failed". It would be clearer to return an empty map when nothing is detected and reserve `null` (or an exception) for actual failures, or introduce a dedicated result type that encodes these states explicitly.
Suggested implementation:
```java
import java.util.Map;
import java.util.HashMap;
import java.util.Collections;
```
```java
* @param workloadName The workload name
* @param namespace The Kubernetes namespace
* @return Map of detected layers (layer name -> KruizeLayer).
* Returns an empty map when no layers are detected or inputs are invalid.
*/
```
```java
if (containerName == null || containerName.isBlank()) {
LOGGER.warn("Container name is null or empty, cannot detect layers");
return Collections.emptyMap();
}
if (namespace == null || namespace.isBlank()) {
LOGGER.warn("Namespace is null or empty, cannot detect layers");
return Collections.emptyMap();
}
```
From your comment, this method also returns `null` for DB failures, "no layers in DB", and "no layers detected" in other branches that are not shown here. To fully implement the suggestion, you should:
1. Replace those other `return null;` statements in `detectLayers` with `return Collections.emptyMap();` where the situation represents "no layers detected/present".
2. Consider differentiating true failures (e.g., DB exceptions) by either:
- Letting exceptions propagate, or
- Wrapping the result in a dedicated result type (e.g., `LayerDetectionResult` with fields like `status`, `layers`, and `errorMessage`) instead of returning `null`.
3. Update any callers that currently rely on `null` to indicate failure so they instead:
- Treat an empty map as "no layers present/detected", and
- Handle exceptions or a new result type for failure cases.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/utils/LayerUtils.java
Outdated
Show resolved
Hide resolved
bharathappali
requested changes
Feb 9, 2026
src/main/java/com/autotune/analyzer/kruizeLayer/presence/PresenceAlways.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/utils/LayerUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/presence/QueryBasedPresence.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/utils/LayerUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/utils/LayerUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/utils/LayerUtils.java
Outdated
Show resolved
Hide resolved
src/main/java/com/autotune/analyzer/kruizeLayer/utils/LayerUtils.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Accommodated all review comments for the pr |
Contributor
|
@bhanvimenghani As discussed, the most common issue with promql queries is that it fails in a variety of unpredictable ways sometimes based on the server that you are connected to. Since there is no one promql query validation library, the best way would be to actually post the query to the server and ensure that it works in the current context or log the error. It is best if we can do this at the time of I'm ok if this can be done in subsequent PRs of this series as long as this is addressed. Please open an issue so that we can keep track of this and we can merge this PR for now |
Contributor
Author
|
@dinogun sure! I do have this added as part of the validation change with me, I can create a new pr for this on top of bharath's changes |
dinogun
approved these changes
Feb 11, 2026
Contributor
dinogun
left a comment
dinogun
left a comment
There was a problem hiding this comment.
query validation to be done in a subsequent PR in this series
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
Please describe the issue or feature and the summary of changes made to fix this.
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 a unified layer presence detection mechanism and utilities for determining active layers for a given container and workload.
New Features:
Enhancements: