Fix Bulk label filtering to avoid returning all workloads#2020
Open
khansaad wants to merge 20 commits into
Open
Fix Bulk label filtering to avoid returning all workloads#2020khansaad wants to merge 20 commits into
khansaad wants to merge 20 commits into
Conversation
Signed-off-by: Pinky Gupta <pinkygupta@Pinkys-MacBook-Pro.local>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
Signed-off-by: Pinky Gupta <pinky.gupta1@ibm.com>
…x-bulk-label-filtering # Conflicts: # manifests/autotune/metadata-profiles/bulk_cluster_metadata_local_monitoring.yaml
Signed-off-by: Saad Khan <saakhan@ibm.com>
Contributor
Reviewer's GuideImplements robust label-based filtering for bulk metadata import so that when label filters match zero workloads, the system returns no metadata instead of all workloads, while adding support for complex label inputs and explicit user feedback. Sequence diagram for bulk label-based metadata import and zero-match handlingsequenceDiagram
participant BulkJobManager
participant DataSourceManager
participant DataSourceMetadataOperator
participant DataSourceMetadataHelper
BulkJobManager->>BulkJobManager: buildResourceFilters(filter.getInclude())
BulkJobManager->>BulkJobManager: buildResourceFilters(filter.getExclude())
BulkJobManager->>DataSourceManager: importMetadataFromDataSource(metadataProfileName, datasource, null, startTime, endTime, steps, measurementDuration, includeResourcesMap, excludeResourcesMap)
DataSourceManager->>DataSourceMetadataOperator: processQueriesAndPopulateDataSourceMetadataInfo(metadataProfileName, dataSourceInfo, startTime, endTime, steps, measurementDuration, includeResources, excludeResources)
DataSourceMetadataOperator->>DataSourceMetadataOperator: getNamespacesMatchingLabelFilter(dataSourceInfo, metadataProfile, includeResources, excludeResources, startTime, endTime, steps, measurementDuration, namespaceQuery, false)
DataSourceMetadataOperator->>DataSourceMetadataOperator: getWorkloadsMatchingPodLabelFilter(dataSourceInfo, metadataProfile, includeResources, excludeResources, startTime, endTime, steps, measurementDuration, workloadQuery, false)
DataSourceMetadataOperator->>DataSourceMetadataOperator: fetchQueryResults(namespaceQuery)
DataSourceMetadataOperator->>DataSourceMetadataHelper: getActiveNamespaces(namespacesDataResultArray)
DataSourceMetadataOperator->>DataSourceMetadataOperator: fetchQueryResults(workloadQuery)
DataSourceMetadataOperator->>DataSourceMetadataHelper: getWorkloadInfo(workloadDataResultArray)
DataSourceMetadataOperator->>DataSourceMetadataOperator: fetchQueryResults(containerQuery)
DataSourceMetadataOperator->>DataSourceMetadataHelper: getContainerInfo(containerDataResultArray)
DataSourceMetadataOperator->>DataSourceMetadataHelper: updateContainerDataSourceMetadataInfoObject(dataSourceName, dataSourceMetadataInfo, datasourceWorkloads, datasourceContainers)
alt [matchedNamespaces and matchedWorkloads empty AND exclude-only filters]
DataSourceMetadataOperator->>DataSourceMetadataOperator: getNamespacesMatchingLabelFilter(..., excludeAsInclude, {}, startTime, endTime, steps, measurementDuration, namespaceQuery, true)
DataSourceMetadataOperator->>DataSourceMetadataOperator: getWorkloadsMatchingPodLabelFilter(..., excludeAsInclude, {}, startTime, endTime, steps, measurementDuration, workloadQuery, true)
end
DataSourceMetadataOperator->>DataSourceMetadataHelper: filterMetadataInfoObject(dataSourceName, dataSourceMetadataInfo, matchedNamespaces, matchedWorkloads)
DataSourceMetadataHelper-->>DataSourceMetadataOperator: filterApplied (true/false)
alt [filterApplied == false]
DataSourceMetadataOperator-->>DataSourceManager: return null
DataSourceManager-->>BulkJobManager: metadataInfo = null
BulkJobManager->>BulkJobManager: labelFilterRequested?
alt [labelFilterRequested]
BulkJobManager->>BulkJobManager: setFinalJobStatus(COMPLETED, HTTP_OK, LABEL_FILTER_NO_MATCH_INFO, datasource)
else [no label filter]
BulkJobManager->>BulkJobManager: setFinalJobStatus(COMPLETED, HTTP_OK, NOTHING_INFO, datasource)
end
else [filterApplied == true]
DataSourceMetadataOperator-->>DataSourceManager: return dataSourceMetadataInfo
DataSourceManager-->>BulkJobManager: metadataInfo
BulkJobManager->>BulkJobManager: getExperimentMap(labelString, jobData, metadataInfo, datasource)
end
File-Level Changes
Possibly linked issues
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 5 issues, and left some high level feedback:
- The new label filtering logic adds a large amount of INFO-level logging in hot paths (e.g.,
getNamespacesMatchingLabelFilter,getWorkloadsMatchingPodLabelFilter,buildLabelFilters,escapePromQLLabelValue); consider downgrading many of these to DEBUG to avoid log noise and performance impact in production. - In
filterMetadataInfoObject, returningtrueafter catching exceptions silently masks failures in label-based filtering; it may be safer to propagate an explicit error or a distinct return value so callers can differentiate between "no filter" and "filter failed" cases. - The change to
BulkInput.Filter.labelsfromMap<String, String>toMap<String, Object>plus@JsonAnySettermodifies how filter JSON is parsed; verify and constrain the accepted value types (e.g., only String or List) and validate them early to avoid unexpected runtime type issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new label filtering logic adds a large amount of INFO-level logging in hot paths (e.g., `getNamespacesMatchingLabelFilter`, `getWorkloadsMatchingPodLabelFilter`, `buildLabelFilters`, `escapePromQLLabelValue`); consider downgrading many of these to DEBUG to avoid log noise and performance impact in production.
- In `filterMetadataInfoObject`, returning `true` after catching exceptions silently masks failures in label-based filtering; it may be safer to propagate an explicit error or a distinct return value so callers can differentiate between "no filter" and "filter failed" cases.
- The change to `BulkInput.Filter.labels` from `Map<String, String>` to `Map<String, Object>` plus `@JsonAnySetter` modifies how filter JSON is parsed; verify and constrain the accepted value types (e.g., only String or List<String>) and validate them early to avoid unexpected runtime type issues.
## Individual Comments
### Comment 1
<location path="src/main/java/com/autotune/common/datasource/DataSourceMetadataOperator.java" line_range="263-272" />
<code_context>
+ LOGGER.info("Include resources: {}", includeResources);
+ LOGGER.info("Exclude resources: {}", excludeResources);
+
+ HashMap<String, DataSourceNamespace> matchedNamespaces = getNamespacesMatchingLabelFilter(dataSourceInfo, metadataProfile, includeResources, excludeResources, startTime, endTime, steps, measurementDuration, namespaceQuery, false);
+ HashMap<String, HashMap<String, DataSourceWorkload>> matchedWorkloads = getWorkloadsMatchingPodLabelFilter(dataSourceInfo, metadataProfile, includeResources, excludeResources, startTime, endTime, steps, measurementDuration, workloadQuery, false);
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Label-filter maps are always non-null, causing unintended filtering even when no label filters are requested.
`matchedNamespaces` and `matchedWorkloads` are always initialized as non-null `HashMap`s. When no label filters are provided, the matching methods return empty maps, but `filterMetadataInfoObject` treats any non-null map as “filter requested.” This makes “no filters” behave like “filters with zero matches,” so `filterMetadataInfoObject` may return `false` and `importMetadataFromDataSource` returns `null` even though the user did not request filtering.
Please adjust this by either passing `null` for `matchedNamespaces`/`matchedWorkloads` when no label filters are present, or by changing `filterMetadataInfoObject` to infer whether filtering is requested from the label filter configuration in `includeResources`/`excludeResources` rather than from `matched* != null`.
</issue_to_address>
### Comment 2
<location path="src/main/java/com/autotune/common/data/dataSourceMetadata/DataSourceMetadataHelper.java" line_range="493-494" />
<code_context>
+ return true;
+ }
+
+ // null = no filter requested, empty HashMap = filter requested but no matches
+ boolean namespaceFilterRequested = matchedNamespaces != null;
+ boolean workloadFilterRequested = matchedWorkloads != null;
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Distinguishing "no filter requested" from "no matches" via null vs empty is brittle given current call sites.
This logic assumes `null` means "no filter requested" and an empty map means "filter requested but no matches", but current callers always pass a non-null map (e.g. `new HashMap<>()`), even when no label filters are configured. As a result, the "filter requested" path runs and may return `false` when both maps are empty, which the caller treats as "no matching resources" and aborts incorrectly. To fix this, either ensure callers pass `null` when no label filters are requested, or derive `namespaceFilterRequested` / `workloadFilterRequested` from whether any label filter inputs were actually provided (e.g., via a separate flag or by checking the original filter maps), rather than `matched* != null`.
</issue_to_address>
### Comment 3
<location path="src/main/java/com/autotune/common/datasource/DataSourceMetadataOperator.java" line_range="436-438" />
<code_context>
+ * @param measurementDuration Measurement duration in minutes
+ * @return HashMap of namespaces matching the include filter and not matching the exclude filter
+ */
+ private HashMap<String, DataSourceNamespace> getNamespacesMatchingLabelFilter(DataSourceInfo dataSourceInfo, MetadataProfile metadataProfile,
+ Map<String, String> includeResources, Map<String, String> excludeResources,
+ long startTime, long endTime, int steps, int measurementDuration, String namespaceQueryForAll, boolean forceQuery) throws IOException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException {
+ HashMap<String, DataSourceNamespace> matchedNamespaces = new HashMap<>();
+ String namespaceLabelFilter = includeResources.getOrDefault("namespaceLabelFilter", "");
</code_context>
<issue_to_address>
**suggestion:** Unused parameter `namespaceQueryForAll` in label filter helpers adds confusion.
These methods accept `namespaceQueryForAll` / `workloadQueryForAll` but never use them, which makes the API misleading and suggests unused behavior (e.g., an "all" fallback). Either remove these parameters or integrate them into the logic if they’re needed, so the method signatures reflect actual behavior.
Suggested implementation:
```java
* @param dataSourceInfo The data source information
* @param metadataProfile The metadata profile containing the query
* @param includeResources Map containing the namespaceLabelFilter for inclusion
* @param excludeResources Map containing the namespaceLabelFilter for exclusion
* @param startTime Start time for the query
* @param endTime End time for the query
* @param steps Query step interval
* @param measurementDuration Measurement duration in minutes
* @return HashMap of namespaces matching the include filter and not matching the exclude filter
*/
private HashMap<String, DataSourceNamespace> getNamespacesMatchingLabelFilter(DataSourceInfo dataSourceInfo, MetadataProfile metadataProfile,
Map<String, String> includeResources, Map<String, String> excludeResources,
long startTime, long endTime, int steps, int measurementDuration, boolean forceQuery) throws IOException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException {
HashMap<String, DataSourceNamespace> matchedNamespaces = new HashMap<>();
String namespaceLabelFilter = includeResources.getOrDefault("namespaceLabelFilter", "");
String excludeNamespaceLabelFilter = excludeResources.getOrDefault("namespaceLabelFilter", "");
LOGGER.info("Namespace label filtering - Include filter: '{}', Exclude filter: '{}'",
namespaceLabelFilter, excludeNamespaceLabelFilter);
if (namespaceLabelFilter.isEmpty() && excludeNamespaceLabelFilter.isEmpty()) {
LOGGER.info("No namespace label filters provided, skipping namespace label filtering");
return matchedNamespaces;
}
```
1. Update all callers of `getNamespacesMatchingLabelFilter` to remove the `namespaceQueryForAll` argument and pass only the remaining parameters.
2. Apply the same cleanup to any similar helper methods that take `namespaceQueryForAll` or `workloadQueryForAll` but do not use them: remove these parameters from their signatures, adjust Javadocs, and update all call sites accordingly.
</issue_to_address>
### Comment 4
<location path="src/main/java/com/autotune/analyzer/workerimpl/BulkJobManager.java" line_range="689" />
<code_context>
+ LOGGER.info("Escaping label value - Original: [{}]", value);
</code_context>
<issue_to_address>
**suggestion (performance):** Escaping helper logs at INFO for each label value, which may be noisy for bulk jobs.
This logs every escaped label value at INFO, which can overwhelm logs for bulk jobs and obscure more important messages. Please move this to DEBUG and, if needed, keep only a single INFO-level summary when building the filter to preserve traceability without adding noise.
</issue_to_address>
### Comment 5
<location path="src/main/java/com/autotune/common/data/dataSourceMetadata/DataSourceMetadataHelper.java" line_range="575-584" />
<code_context>
+ LOGGER.info("Looking for metric '{}' in profile with {} metrics", metricName, metrics != null ? metrics.size() : 0);
</code_context>
<issue_to_address>
**suggestion:** Verbose INFO-level logging in `getQueryFromProfile` may impact log clarity and volume.
Since `getQueryFromProfile` is likely invoked for many queries, logging each metric lookup at `INFO` can flood production logs and repeatedly expose full query details. Consider moving the detailed metric/query logging to `DEBUG` and keeping `INFO` for higher-level events (such as missing metrics) to preserve log signal-to-noise ratio.
Suggested implementation:
```java
public String getQueryFromProfile(MetadataProfile metadataProfile, String metricName) {
List<Metric> metrics = metadataProfile.getQueryVariables();
LOGGER.debug("Looking for metric '{}' in profile with {} metrics", metricName, metrics != null ? metrics.size() : 0);
if (metrics == null || metrics.isEmpty()) {
LOGGER.warn("No metrics found in metadata profile");
return null;
}
// Log all available metric names for debugging
LOGGER.debug("Available metrics in profile: {}",
metrics.stream().map(Metric::getName).collect(java.util.stream.Collectors.joining(", ")));
```
1. If there are any other log statements in `getQueryFromProfile` that include full query text or per-metric details at `INFO` level, consider downgrading them to `DEBUG` as well to keep `INFO` focused on higher-level events (e.g., missing metrics, configuration issues).
2. Optionally, you can add a single `INFO` log summarizing when a metric is not found (if not already present later in the method), while keeping successful lookups and detailed query contents at `DEBUG`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+436
to
+438
| private HashMap<String, DataSourceNamespace> getNamespacesMatchingLabelFilter(DataSourceInfo dataSourceInfo, MetadataProfile metadataProfile, | ||
| Map<String, String> includeResources, Map<String, String> excludeResources, | ||
| long startTime, long endTime, int steps, int measurementDuration, String namespaceQueryForAll, boolean forceQuery) throws IOException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException { |
Contributor
There was a problem hiding this comment.
suggestion: Unused parameter namespaceQueryForAll in label filter helpers adds confusion.
These methods accept namespaceQueryForAll / workloadQueryForAll but never use them, which makes the API misleading and suggests unused behavior (e.g., an "all" fallback). Either remove these parameters or integrate them into the logic if they’re needed, so the method signatures reflect actual behavior.
Suggested implementation:
* @param dataSourceInfo The data source information
* @param metadataProfile The metadata profile containing the query
* @param includeResources Map containing the namespaceLabelFilter for inclusion
* @param excludeResources Map containing the namespaceLabelFilter for exclusion
* @param startTime Start time for the query
* @param endTime End time for the query
* @param steps Query step interval
* @param measurementDuration Measurement duration in minutes
* @return HashMap of namespaces matching the include filter and not matching the exclude filter
*/
private HashMap<String, DataSourceNamespace> getNamespacesMatchingLabelFilter(DataSourceInfo dataSourceInfo, MetadataProfile metadataProfile,
Map<String, String> includeResources, Map<String, String> excludeResources,
long startTime, long endTime, int steps, int measurementDuration, boolean forceQuery) throws IOException, NoSuchAlgorithmException, KeyStoreException, KeyManagementException {
HashMap<String, DataSourceNamespace> matchedNamespaces = new HashMap<>();
String namespaceLabelFilter = includeResources.getOrDefault("namespaceLabelFilter", "");
String excludeNamespaceLabelFilter = excludeResources.getOrDefault("namespaceLabelFilter", "");
LOGGER.info("Namespace label filtering - Include filter: '{}', Exclude filter: '{}'",
namespaceLabelFilter, excludeNamespaceLabelFilter);
if (namespaceLabelFilter.isEmpty() && excludeNamespaceLabelFilter.isEmpty()) {
LOGGER.info("No namespace label filters provided, skipping namespace label filtering");
return matchedNamespaces;
}
- Update all callers of
getNamespacesMatchingLabelFilterto remove thenamespaceQueryForAllargument and pass only the remaining parameters. - Apply the same cleanup to any similar helper methods that take
namespaceQueryForAllorworkloadQueryForAllbut do not use them: remove these parameters from their signatures, adjust Javadocs, and update all call sites accordingly.
Comment on lines
+575
to
+584
| LOGGER.info("Looking for metric '{}' in profile with {} metrics", metricName, metrics != null ? metrics.size() : 0); | ||
|
|
||
| if (metrics == null || metrics.isEmpty()) { | ||
| LOGGER.warn("No metrics found in metadata profile"); | ||
| return null; | ||
| } | ||
|
|
||
| // Log all available metric names for debugging | ||
| LOGGER.info("Available metrics in profile: {}", | ||
| metrics.stream().map(Metric::getName).collect(java.util.stream.Collectors.joining(", "))); |
Contributor
There was a problem hiding this comment.
suggestion: Verbose INFO-level logging in getQueryFromProfile may impact log clarity and volume.
Since getQueryFromProfile is likely invoked for many queries, logging each metric lookup at INFO can flood production logs and repeatedly expose full query details. Consider moving the detailed metric/query logging to DEBUG and keeping INFO for higher-level events (such as missing metrics) to preserve log signal-to-noise ratio.
Suggested implementation:
public String getQueryFromProfile(MetadataProfile metadataProfile, String metricName) {
List<Metric> metrics = metadataProfile.getQueryVariables();
LOGGER.debug("Looking for metric '{}' in profile with {} metrics", metricName, metrics != null ? metrics.size() : 0);
if (metrics == null || metrics.isEmpty()) {
LOGGER.warn("No metrics found in metadata profile");
return null;
}
// Log all available metric names for debugging
LOGGER.debug("Available metrics in profile: {}",
metrics.stream().map(Metric::getName).collect(java.util.stream.Collectors.joining(", ")));
- If there are any other log statements in
getQueryFromProfilethat include full query text or per-metric details atINFOlevel, consider downgrading them toDEBUGas well to keepINFOfocused on higher-level events (e.g., missing metrics, configuration issues). - Optionally, you can add a single
INFOlog summarizing when a metric is not found (if not already present later in the method), while keeping successful lookups and detailed query contents atDEBUG.
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 updates logic to not return any workloads in case the label filter returns zero workloads.
Note: This is on top of #1918
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
Improve bulk job workload filtering by applying namespace and pod label filters at the metadata import stage and returning no workloads when filters match nothing.
Bug Fixes:
Enhancements: