feat(ddm): Allow multiple use case ids to be queried at once and parallelize #66298
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
github-actions
bot
added
the
Scope: Backend
iambriccardo
commented
Mar 5, 2024
| request = Request( | ||
| dataset=Dataset.Metrics.value, | ||
| dataset=Dataset.Metrics.value |
There was a problem hiding this comment.
For some reason, the wrong dataset was used but we still got data back, interesting.
iambriccardo
commented
Mar 5, 2024
| break | ||
|
|
||
| stored_metrics = get_stored_metrics_of_projects(projects, use_case_ids, start, end) | ||
| metrics_blocking_state = ( |
There was a problem hiding this comment.
Decided to move out the check for the use case, since it's not a responsibility of get_metrics_blocking_state_of_projects since it should be use case agnostic.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #66298 +/- ##
==========================================
- Coverage 84.29% 84.29% -0.01%
==========================================
Files 5306 5306
Lines 237101 237095 -6
Branches 41031 41033 +2
==========================================
- Hits 199866 199859 -7
- Misses 37016 37017 +1
Partials 219 219
|
obostjancic
approved these changes
Mar 5, 2024
| start: datetime | None = None, | ||
| end: datetime | None = None, | ||
| ) -> Sequence[MetricMeta]: | ||
| if not projects: | ||
| return [] | ||
|
|
||
| stored_metrics = get_stored_metrics_of_projects(projects, use_case_id, start, end) | ||
| metrics_blocking_state = get_metrics_blocking_state_of_projects(projects, use_case_id) | ||
| has_custom_use_case_id = False |
There was a problem hiding this comment.
Suggested change
| has_custom_use_case_id = False | |
| has_custom_use_case_id = UseCaseID.CUSTOM in use_case_ids |
There was a problem hiding this comment.
Does it work for lists? I will check now
There was a problem hiding this comment.
ofc, python is pseudo code 😅
Comment on lines
313
to
334
| entity_keys = defaultdict(set) | ||
| for use_case_id in use_case_ids: | ||
| entity_keys[use_case_id] = entity_keys[use_case_id].union( | ||
| get_entity_keys_of_use_case_id(use_case_id=use_case_id) | ||
| ) | ||
|
|
||
| grouped_stored_metrics = {} | ||
| for stored_metric in stored_metrics: | ||
| grouped_stored_metrics.setdefault(stored_metric["metric_id"], []).append( | ||
| stored_metric["project_id"] | ||
| ) | ||
| # We compute a list of all the queries that we want to run in parallel across entities and use cases. | ||
| requests = [] | ||
| use_case_id_to_index = defaultdict(list) | ||
| for use_case_id, entity_keys in entity_keys.items(): | ||
| for entity_key in entity_keys: | ||
| requests.append( | ||
| _get_metrics_by_project_for_entity_query( | ||
| entity_key=entity_key, | ||
| project_ids=project_ids, | ||
| org_id=org_id, | ||
| use_case_id=use_case_id, | ||
| start=start, | ||
| end=end, | ||
| ) | ||
| ) | ||
| use_case_id_to_index[use_case_id].append(len(requests) - 1) |
There was a problem hiding this comment.
nit: i would do these two things in the same nested loops
There was a problem hiding this comment.
computing entity_keys dict and then computing a list of requests
There was a problem hiding this comment.
I did put them separate to make the implementation work even if you pass two use case ids and if entity keys are different, but it's a bit overengineered. I can do like you said, will simplify! Thanks for the suggestion
Comment on lines
54
to
69
| # mris = get_stored_metrics_of_projects([self.project], [UseCaseID.TRANSACTIONS]) | ||
| # assert mris == { | ||
| # "d:transactions/duration@millisecond": [self.project.id], | ||
| # } | ||
| # | ||
| # mris = get_stored_metrics_of_projects([self.project], [UseCaseID.SESSIONS]) | ||
| # assert mris == { | ||
| # "d:sessions/duration@second": [self.project.id], | ||
| # "c:sessions/session@none": [self.project.id], | ||
| # "s:sessions/user@none": [self.project.id], | ||
| # } | ||
| # | ||
| # mris = get_stored_metrics_of_projects([self.project], [UseCaseID.CUSTOM]) | ||
| # assert mris == { | ||
| # custom_mri: [self.project.id], | ||
| # } |
There was a problem hiding this comment.
Suggested change
| # mris = get_stored_metrics_of_projects([self.project], [UseCaseID.TRANSACTIONS]) | |
| # assert mris == { | |
| # "d:transactions/duration@millisecond": [self.project.id], | |
| # } | |
| # | |
| # mris = get_stored_metrics_of_projects([self.project], [UseCaseID.SESSIONS]) | |
| # assert mris == { | |
| # "d:sessions/duration@second": [self.project.id], | |
| # "c:sessions/session@none": [self.project.id], | |
| # "s:sessions/user@none": [self.project.id], | |
| # } | |
| # | |
| # mris = get_stored_metrics_of_projects([self.project], [UseCaseID.CUSTOM]) | |
| # assert mris == { | |
| # custom_mri: [self.project.id], | |
| # } |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
aliu3ntry
pushed a commit
that referenced
this pull request
Mar 6, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR implements a new fully parallelized implementation of fetching metrics meta. The need for such implementation arose for two reasons:
The new implementation parallelizes all queries for fetching data across entities and use case ids. It also maximizes parallelization when reverse resolving metric ids.
Closes: #66126