Skip to content

Comments

fix: added default value to time,space aggregation to fix query_range getting 500 for metric#7414

Merged
SagarRajput-7 merged 7 commits intomainfrom
SIG-7395
Mar 28, 2025
Merged

fix: added default value to time,space aggregation to fix query_range getting 500 for metric#7414
SagarRajput-7 merged 7 commits intomainfrom
SIG-7395

Conversation

@SagarRajput-7
Copy link
Contributor

@SagarRajput-7 SagarRajput-7 commented Mar 24, 2025

Summary

We are getting 500 instead of No Data, when firing the stage and run with an aggregate filter that we have not received/setup yet.

This is happening because timeAggregation and spaceAggregation are empty, that is because:

Here in this case the aggregate filter that we are adding is from a metric that we haven't received yet, hence it won't have any metadata aggregateAttribute like datatype, type, key etc.
Under this PR - #4569 we added a handling that sets aggregateOperator, timeAggregation and spaceAggregation based on the aggregateAttribute type (which in this case is empty ("").
Hence it fallbacks to defaults, which in this case are empty.
(this handling is specifically done for metric only)

Fix:

Added default values - Avg (for time), Avg (for space) and also Avg (for operator)

Related Issues / PR's

Screenshots

Before:

Screen.Recording.2025-03-24.at.9.47.10.AM.mov

After:

Screen.Recording.2025-03-25.at.6.35.05.PM.mov

Affected Areas and Manually Tested Areas


Important

Fixes 500 error in metric queries by setting default time and space aggregations when attributes are empty.

  • Behavior:
    • Fixes 500 error in query_range by setting default timeAggregation to 'Rate' and spaceAggregation to 'Sum' when empty.
    • Affects queries with metrics that lack metadata aggregate attributes.
  • Code Changes:
    • Adds metricsEmptySpaceAggregateOperatorOptions in queryBuilderOperators.ts with default operators.
    • Modifies handleMetricAggregateAtributeTypes in useQueryBuilderOperations.ts to use default operators when attribute type is empty.
    • Updates handleChangeAggregatorAttribute in useQueryBuilderOperations.ts to set default aggregations.

This description was created by Ellipsis for 86d2109. It will automatically update as commits are pushed.

@SagarRajput-7 SagarRajput-7 requested a review from YounixM as a code owner March 24, 2025 04:20
@github-actions github-actions bot added the bug Something isn't working label Mar 24, 2025
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to c5a36a1 in 1 minute and 13 seconds

More details
  • Looked at 23 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:197
  • Draft comment:
    For ATTRIBUTE_TYPES.SUM, assigning spaceAggregation to MetricAggregateOperator.SUM is clear. Ensure that using RATE for timeAggregation and SUM for spaceAggregation aligns with business logic.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the author to ensure that the use of RATE for timeAggregation and SUM for spaceAggregation aligns with business logic. This is a request for confirmation of intention, which is not allowed according to the rules. Therefore, this comment should be removed.
2. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:201
  • Draft comment:
    For ATTRIBUTE_TYPES.GAUGE, using AVG consistently for both time and space aggregation looks appropriate. Confirm consistency with metric definitions.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
3. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:203
  • Draft comment:
    In the default case, defaulting to MetricAggregateOperator.RATE for timeAggregation and MetricAggregateOperator.SUM for spaceAggregation replaces the empty string assignment. Verify that these defaults are correct for all non-SUM/GUAGE types.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to verify if the defaults are correct for all non-SUM/GUAGE types. This falls under asking the author to confirm their intention or to double-check things, which is against the rules.
4. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:196
  • Draft comment:
    For SUM, setting spaceAggregation to MetricAggregateOperator.SUM prevents empty values. Confirm that this is the intended default for SUM-type queries.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
5. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:201
  • Draft comment:
    For GAUGE attributes, spaceAggregation is set to AVG. Verify that using the same operator (AVG) for both time and space aggregation meets the requirements.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to verify if using the same operator for both time and space aggregation meets the requirements. This falls under asking the author to confirm their intention or to double-check things, which is against the rules.
6. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:204
  • Draft comment:
    In the default branch, timeAggregation and spaceAggregation now default to RATE and SUM respectively. Ensure these defaults are appropriate for non-SUM/GAUGE attribute types, as this change prevents empty values (which were causing 500 errors).
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the author to ensure that the defaults are appropriate for certain attribute types. This falls under asking the author to ensure behavior is intended, which is against the rules. Therefore, this comment should not be approved.
7. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:38
  • Draft comment:
    Typographical error: The function name 'getFormatedLegend' appears to be misspelled. Consider renaming it to 'getFormattedLegend' to follow standard spelling.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:146
  • Draft comment:
    Typographical error: The function name 'handleMetricAggregateAtributeTypes' is misspelled. It should be 'handleMetricAggregateAttributeTypes' (note the missing 't' in 'Atribute').
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_ed3fqyYvpAiDvsFH


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@srikanthccv
Copy link
Member

@SagarRajput-7, when we don't know the metric type we show all possible time and space aggregations (superset of all types) and let the user choose the relevant options.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on e02aa73 in 38 seconds

More details
  • Looked at 23 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:203
  • Draft comment:
    Defaulting to RATE for timeAggregation and SUM for spaceAggregation in the else branch seems intended. Confirm that these defaults correctly reflect the business logic when aggregateAttribute.type is not SUM or GAUGE.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:196
  • Draft comment:
    Good use of default aggregation values in the SUM and GAUGE cases. Ensure consistency with backend expectations.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:196
  • Draft comment:
    Good: For ATTRIBUTE_TYPES.SUM, spaceAggregation is now explicitly set to MetricAggregateOperator.SUM, ensuring consistency with timeAggregation.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:200
  • Draft comment:
    For ATTRIBUTE_TYPES.GAUGE, spaceAggregation is now set to AVG, which mirrors the timeAggregation; this improves consistency.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:203
  • Draft comment:
    In the default branch, timeAggregation and spaceAggregation are now set to RATE and SUM respectively. Consider if aggregateOperator also needs a default value for unsupported types, to ensure backend queries receive complete aggregation data.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:146
  • Draft comment:
    Typo: The function name 'handleMetricAggregateAtributeTypes' appears to have a misspelling. It should likely be 'handleMetricAggregateAttributeTypes' to maintain consistency with the rest of the code (e.g., 'aggregateAttribute').
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:38
  • Draft comment:
    Typo: The function name 'getFormatedLegend' is likely intended to be 'getFormattedLegend'. Consider correcting the spelling for clarity and consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_7xAp7igLYKPBpCGo


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on be7e92c in 1 minute and 20 seconds

More details
  • Looked at 101 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 8 drafted comments based on config settings.
1. frontend/src/constants/queryBuilderOperators.ts:429
  • Draft comment:
    Ensure the default space aggregation options fall back to these values when metadata is missing. Also, if there are design tokens or centralized constants for labels, consider using them instead of hardcoded strings.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
2. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:150
  • Draft comment:
    In handleMetricAggregateAtributeTypes, the fallback to metricAggregateOperatorOptions when aggregateAttribute.type is empty is appropriate. Ensure that this default behavior (and later default assignment of RATE and SUM in handleChangeAggregatorAttribute) fully addresses the 500 error scenario.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:208
  • Draft comment:
    In the default branch of handleChangeAggregatorAttribute, timeAggregation is set to RATE and spaceAggregation to SUM. Confirm that these defaults are intended for cases when the aggregateAttribute type is missing.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:194
  • Draft comment:
    The PR description mentions a contrast issue with the light/dark mode toggle button, but the code changes only adjust default aggregation values to address a query_range 500 error. Please update the PR description to accurately reflect the changes being introduced.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:149
  • Draft comment:
    Typo: The function name 'handleMetricAggregateAtributeTypes' is misspelled. Consider renaming it to 'handleMetricAggregateAttributeTypes' for clarity.
  • Reason this comment was not posted:
    Comment was on unchanged code.
6. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:43
  • Draft comment:
    Note: The PR title and commit message indicate a fix for default aggregation values (to avoid 500 errors) but the issue description mentions a contrast problem with the toggle button. Please verify that the intended bug is being addressed.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:149
  • Draft comment:
    Typo detected: The function name 'handleMetricAggregateAtributeTypes' is misspelled. Consider renaming it to 'handleMetricAggregateAttributeTypes' for clarity.
  • Reason this comment was not posted:
    Comment was on unchanged code.
8. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:41
  • Draft comment:
    Typo suggestion: The imported function 'getFormatedLegend' may be intended to be spelled as 'getFormattedLegend'. If possible, consider updating the function name for consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_sYGVFzyl44wSeKPQ


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 86d2109 in 2 minutes and 10 seconds

More details
  • Looked at 19 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:199
  • Draft comment:
    The removed explicit check for ATTRIBUTE_TYPES.GAUGE changes the default behavior. Now, only ATTRIBUTE_TYPES.SUM gets RATE/SUM and all other cases (including GAUGE and empty types) default to AVG/AVG. Please ensure this aligns with the intent described in the PR (default should be RATE for time and SUM for space) and that no use-case is affected.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:200
  • Draft comment:
    The new branch now sets all values to AVG in the fallback case (i.e. when aggregateAttribute.type is not SUM), whereas the PR description indicates that the defaults should be RATE for time and SUM for space. In the original code, fallback (for types other than SUM and GAUGE) used RATE/SUM. Please verify if the intended fallback for a missing or non‐SUM type should indeed be AVG, or if it should remain RATE/SUM as described.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
    The comment raises a valid point about a behavioral change. The code clearly shows that the defaults for non-SUM types have changed. However, the comment asks for verification rather than pointing out a clear issue. It's phrased as a request to check intentions rather than identifying a concrete problem. The rules specifically say not to ask authors to verify their intentions.
    The change could be intentional refactoring to simplify the logic. Without access to the PR description or requirements, I can't be certain this is actually a problem.
    While the behavior change is real, the comment violates the rule about not asking authors to verify their intentions. If this was an actual issue, it should be phrased as a clear problem statement.
    The comment should be deleted because it's phrased as a verification request rather than identifying a clear issue, which violates our commenting rules.
3. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:150
  • Draft comment:
    Typo: The function name 'handleMetricAggregateAtributeTypes' appears to have a misspelling. Consider renaming it to 'handleMetricAggregateAttributeTypes' for clarity and consistency.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. frontend/src/hooks/queryBuilder/useQueryBuilderOperations.ts:41
  • Draft comment:
    Potential typo in function name 'getFormatedLegend'. If the naming is not intentional or established in the project, consider renaming it to 'getFormattedLegend' for improved readability.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_uYXZzQZYa1krNwG8


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@SagarRajput-7 SagarRajput-7 enabled auto-merge (squash) March 28, 2025 04:50
@SagarRajput-7 SagarRajput-7 merged commit 8c4c357 into main Mar 28, 2025
12 of 14 checks passed
@SagarRajput-7 SagarRajput-7 deleted the SIG-7395 branch March 28, 2025 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants