feat: added series aggregation for group by with value type panel | 5949 #6744
+242
−29
Conversation
This file contains 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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 3d21b6a in 1 minute and 28 seconds
More details
- Looked at
299lines of code in6files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. pkg/query-service/app/metrics/v4/helpers/series_agg_helper.go:28
- Draft comment:
Handle the case whereseriesAggregatorisSeriesAggregationUnspecifiedto avoid unexpected behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment raises a valid point about missing case handling. However, returning the unmodified query when no aggregation is specified could be the intended behavior. Without seeing the SeriesAggregation enum definition or understanding the broader system design, we can't be certain that this is actually a problem. The comment is somewhat speculative.
We don't know if returning the unmodified query for unspecified aggregation is actually a bug - it could be the desired behavior. We also don't have context about how this function is called.
The comment is speculative and asks for defensive programming without clear evidence that it's needed. Following our rules, we should avoid speculative "what if" comments.
Delete the comment as it's speculative and we don't have strong evidence that handling the unspecified case differently is actually required.
2. pkg/query-service/app/metrics/v4/query_builder.go:93
- Draft comment:
Ensure thatAddSeriesAggregationhandles all possible values ofSeriesAggregationto prevent unexpected behavior. - Reason this comment was not posted:
Confidence changes required:50%
TheAddSeriesAggregationfunction is being used inPrepareMetricQueryto modify the query when the panel type isPanelTypeValueand there is a group by clause. This is consistent with the PR description and the intended functionality.
3. pkg/query-service/app/metrics/v4/query_builder_test.go:618
- Draft comment:
The test cases forPrepareMetricQueryare comprehensive and cover the new series aggregation feature effectively. - Reason this comment was not posted:
Confidence changes required:20%
The test cases forPrepareMetricQuerycover various scenarios including the new series aggregation feature. This ensures that the new functionality is well-tested.
4. pkg/query-service/model/v3/v3.go:715
- Draft comment:
TheSeriesAggregationtype and its validation method are well-defined, ensuring only valid values are used. - Reason this comment was not posted:
Confidence changes required:10%
TheSeriesAggregationtype and its constants are defined correctly, and theValidatemethod ensures that only valid values are used. This aligns with the new feature implementation.
5. pkg/query-service/app/metrics/v4/helpers/series_agg_helper.go:1
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Comment was on unchanged code.
6. pkg/query-service/app/metrics/v4/query_builder.go:89
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Marked as duplicate.
7. pkg/query-service/app/metrics/v4/query_builder_test.go:614
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Marked as duplicate.
8. pkg/query-service/app/queryBuilder/query_builder.go:425
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Marked as duplicate.
9. pkg/query-service/app/queryBuilder/query_builder_test.go:1304
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Marked as duplicate.
10. pkg/query-service/model/v3/v3.go:712
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_t8xPXQ8UHha4Q4T4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
@aniketio-ctrl Let's call it |
done bro |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 421a6c2 in 1 minute and 44 seconds
More details
- Looked at
233lines of code in6files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/app/metrics/v4/helpers/series_agg_helper.go:28
- Draft comment:
Consider adding a default case to handleSecondaryAggregationUnspecifiedto avoid returning an unmodified query when the aggregation type is unspecified. - Reason this comment was not posted:
Comment was on unchanged code.
2. pkg/query-service/app/metrics/v4/query_builder.go:92
- Draft comment:
Before callingAddSecondaryAggregation, check ifmq.SecondaryAggregationis notSecondaryAggregationUnspecifiedto avoid unnecessary query modifications. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests adding defensive programming, but without seeing the implementation of AddSecondaryAggregation, we don't know if this check is necessary. The function might already handle unspecified aggregations internally. Also, this seems like a speculative suggestion rather than a clear issue.
I might be wrong about the internal implementation of AddSecondaryAggregation - maybe it really does need this check. Also, defensive programming can prevent bugs.
While defensive programming is good, without seeing the AddSecondaryAggregation implementation, this is a speculative comment. The function should handle invalid inputs internally.
This comment should be deleted as it's speculative and we don't have enough context to know if the check is actually needed.
Workflow ID: wflow_2aaiQ56qmmU8AWzw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
srikanthccv
reviewed
Jan 8, 2025
pkg/query-service/model/v3/v3.go
Outdated
Comment on lines
984
to
988
| if panelType == PanelTypeValue { | ||
| if err := b.SecondaryAggregation.Validate(); err != nil { | ||
| return fmt.Errorf("series aggregation is required for value type panel with group by: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The panels are going to throw an error because you are making it mandatory to have a valid aggregation input for secondary aggregation for value panel types, please make it optional
There was a problem hiding this comment.
but its inside if b.GroupBy != nil {} so only if groupBY is not nil and value type is panel, then only it will validate
There was a problem hiding this comment.
grouby can be a non-nil but empty list.
There was a problem hiding this comment.
added the panelType == PanelTypeValue && len(b.GroupBy) > 0 check
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on fdcf3f9 in 47 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. pkg/query-service/model/v3/v3.go:986
- Draft comment:
The error message should refer to 'secondary aggregation' instead of 'series aggregation' for clarity.
return fmt.Errorf("secondary aggregation is required for value type panel with group by: %w", err)
- Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_m980SwpDf5tYr3kh
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Group by was not possible with value type panel, due to result containing multiple series, we have introduced seriesAggregation for aggregating multiple series into one
Related Issues / PR's
#5949
Affected Areas and Manually Tested Areas
Caching keys in value type panel, and group by with value type panel for metric builder queries.
Important
Adds series aggregation for value type panels with group by in metrics queries, updating query preparation and cache key generation.
AddSecondaryAggregation()inseries_agg_helper.goto aggregate multiple series into one for value type panels.PrepareMetricQuery()inquery_builder.goto apply series aggregation whenPanelTypeValueandGroupByare used.SeriesAggregationtype and constants inv3.gofor sum, avg, min, and max operations.SeriesAggregationfield toBuilderQueryinv3.go.TestPrepareMetricQueryValueTypePanelWithGroupBY()inquery_builder_test.goto test new series aggregation functionality.TestGenerateCacheKeysMetricsBuilder()inquery_builder_test.goto include series aggregation in cache keys.This description was created by
for fdcf3f9. It will automatically update as commits are pushed.