[COST-7430] Enhance Azure report query handling with subscription name fix#6037
Open
[COST-7430] Enhance Azure report query handling with subscription name fix#6037
Conversation
…e ordering - Added a new test to validate the execution of ranked queries with ordering by subscription_name, ensuring that subscription_name is included in the data rows. - Updated the ReportQueryHandler to handle cases where rank_group_by may include dimensions not selected in the main queryset, improving the accuracy of data merging. - Refactored the AzureReportQueryHandler to simplify the conditional logic for annotating subscription_name based on query_group_by. This change improves the flexibility and correctness of Azure report queries, particularly in scenarios involving subscription-based data.
lcouzens
added
the
smoke-tests
github-actions
Bot
added
the
smokes-required
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request simplifies the annotation logic for subscription_name in Azure report query handlers and updates the _ranked_list function to handle dimensions that may not be present in the main queryset. A new test case is introduced to verify ranked queries ordered by subscription_name. Feedback suggests improving this test by explicitly asserting that subscription_name is included in the output data.
Comment on lines
+789
to
+796
| def test_execute_query_limit_orderby_subscription_name(self): | ||
| """Ranked query with order_by subscription_name must include subscription_name on data rows.""" | ||
| url = "?filter[time_scope_units]=month&filter[time_scope_value]=-1&filter[resolution]=monthly&filter[limit]=5&order_by[subscription_name]=asc&group_by[subscription_guid]=*" # noqa: E501 | ||
| path = reverse("reports-azure-costs") | ||
| query_params = self.mocked_query_params(url, AzureCostView, path) | ||
| handler = AzureReportQueryHandler(query_params) | ||
| query_output = handler.execute_query() | ||
| self.assertIsNotNone(query_output.get("data")) |
Contributor
There was a problem hiding this comment.
The test test_execute_query_limit_orderby_subscription_name currently only verifies that the data is not None. Since this PR specifically fixes a KeyError and ensures subscription_name is correctly handled in ranked queries (via the changes in _ranked_list), the test should explicitly verify that subscription_name is present in the resulting data rows. This provides better coverage and ensures the fix works as intended for ranked results.
Suggested change
| def test_execute_query_limit_orderby_subscription_name(self): | |
| """Ranked query with order_by subscription_name must include subscription_name on data rows.""" | |
| url = "?filter[time_scope_units]=month&filter[time_scope_value]=-1&filter[resolution]=monthly&filter[limit]=5&order_by[subscription_name]=asc&group_by[subscription_guid]=*" # noqa: E501 | |
| path = reverse("reports-azure-costs") | |
| query_params = self.mocked_query_params(url, AzureCostView, path) | |
| handler = AzureReportQueryHandler(query_params) | |
| query_output = handler.execute_query() | |
| self.assertIsNotNone(query_output.get("data")) | |
| def test_execute_query_limit_orderby_subscription_name(self): | |
| """Ranked query with order_by subscription_name must include subscription_name on data rows.""" | |
| url = "?filter[time_scope_units]=month&filter[time_scope_value]=-1&filter[resolution]=monthly&filter[limit]=5&order_by[subscription_name]=asc&group_by[subscription_guid]=*" # noqa: E501 | |
| path = reverse("reports-azure-costs") | |
| query_params = self.mocked_query_params(url, AzureCostView, path) | |
| handler = AzureReportQueryHandler(query_params) | |
| query_output = handler.execute_query() | |
| data = query_output.get("data") | |
| self.assertIsNotNone(data) | |
| for data_item in data: | |
| for sub_item in data_item.get("subscription_guids", []): | |
| self.assertIn("subscription_name", sub_item.get("values", [{}])[0]) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6037 +/- ##
=======================================
- Coverage 94.3% 94.3% -0.0%
=======================================
Files 361 361
Lines 31805 31808 +3
Branches 3484 3485 +1
=======================================
Hits 30007 30007
+ Misses 1170 1169 -1
- Partials 628 632 +4 🚀 New features to boost your workflow:
|
- Added new tests to validate the execution of queries in AzureReportQueryHandler and OCPAzureReportQueryHandler, ensuring that subscription names are included in the returned data when ordered by subscription_name. - Improved assertions to check the structure and types of data returned, enhancing test coverage for scenarios involving subscription-based data. These changes improve the robustness of the report query handling for Azure and OCP Azure, ensuring accurate data representation in the results.
Contributor
|
/retest |
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.
The following addresses alert seen in stage: KeyError: 'subscription_name'
This change improves the flexibility and correctness of Azure report queries, particularly in scenarios involving subscription-based data.
Jira Ticket
COST-####
Description
This change will ...
Testing
Release Notes