ref(releasehealth): Implement check_releases_have_health_data on metrics backend [INGEST-249] #28823
Conversation
Co-authored-by: Markus Unterwaditzer <markus-honeypot@unterwaditzer.net>
Co-authored-by: Markus Unterwaditzer <markus-honeypot@unterwaditzer.net>
| ) | ||
|
|
||
| def extract_row_info(row: Mapping[str, Union[OrganizationId, str]]) -> ReleaseName: | ||
| return reverse_tag_value(organization_id, row.get(release_column_name)) # type: ignore |
There was a problem hiding this comment.
Adding here a # type: ignore is unnecessary imo.. I'd much rather not have ReleaseName as a type and return a str here since ReleaseName is just basically an str
There was a problem hiding this comment.
Please # type: ignore comments should not be used liberally, and there must be a good reason to actually add them
There was a problem hiding this comment.
Sure, #type: ignore should not be used liberally, it is necessary here to avoid error generated by row.get():
Argument 2 to "reverse_tag_value" has incompatible type "Union[int, str, None]"; expected "int"
Regarding the point on ReleaseName I disagree though. Any way ReleaseName is used in more APIs.
| @@ -971,6 +971,10 @@ def store_session(self, session): | |||
| if status != "ok": # terminal | |||
| self._push_metric(session, "distribution", "session.duration", {}, session["duration"]) | |||
|
|
|||
| def bulk_store_sessions(self, sessions): | |||
| for session in sessions: | |||
There was a problem hiding this comment.
Nit: self.store(session) for session in sessions
There was a problem hiding this comment.
Not sure what are you suggesting ?
are you suggesting:
def bulk_store_sessions(self, sessions):
[self.store_session(session) for session in sessions ]
That would generate a list for no good reason
or:
def bulk_store_sessions(self, sessions):
(self.store_session(session) for session in sessions)
that would create an iterator that would not do anything (since it is not iterated over).
There was a problem hiding this comment.
Oh nvm, ignore this plz.. misread, thought you were returning a list
| @@ -568,7 +567,7 @@ def test_fetching_release_sessions_time_bounds_for_different_release_with_no_ses | |||
|
|
|||
| class SnubaSessionsTestMetrics(ReleaseHealthMetricsTestCase, SnubaSessionsTest): | |||
| """ | |||
| Same tests as in SnunbaSessionsTest but using the Metrics backendx | |||
| Same tests as in SnunbaSessionsTest but using the Metrics backend | |||
| """ | |||
|
|
|||
| pass | |||
There was a problem hiding this comment.
Does this indicate that tests will be added in the future for metrics? If so I would clarify that with a ToDo comment
There was a problem hiding this comment.
This class runs the same tests with SnubaSessionsTest from which is derived but with the metrics backend due to it being derived from ReleaseHealthMetricsTestCase , no further code needed
There was a problem hiding this comment.
Oh okay, I did not get that from the comment .. I thought you were just using the class as a placeholder to write future tests.. maybe Runs the same tests as in SnubaSessionsTestClass but using the Metrics backend
…ics backend [INGEST-249] (#28823) Implement metrics version for check_releases_have_health_data [INGEST-249]
Add check_releases_have_health_data to the metrics backend