-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ref(releasehealth): Implement check_releases_have_health_data on metrics backend [INGEST-249] #28823
ref(releasehealth): Implement check_releases_have_health_data on metrics backend [INGEST-249] #28823
Changes from all commits
a35bd3b
acc3ed5
ce328b9
7f5ee26
c67d059
bb110e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what are you suggesting ?
That would generate a list for no good reason or:
that would create an iterator that would not do anything (since it is not iterated over). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh nvm, ignore this plz.. misread, thought you were returning a list |
||
self.store_session(session) | ||
|
||
@classmethod | ||
def _push_metric(cls, session, type, name, tags, value): | ||
def metric_id(name): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,6 @@ | |
from sentry.releasehealth.sessions import SessionsReleaseHealthBackend | ||
from sentry.snuba.sessions import ( | ||
_make_stats, | ||
check_releases_have_health_data, | ||
get_adjacent_releases_based_on_adoption, | ||
get_oldest_health_data_for_releases, | ||
get_project_releases_by_stability, | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this indicate that tests will be added in the future for metrics? If so I would clarify that with a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class runs the same tests with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
@@ -1853,13 +1852,15 @@ def test(self): | |
|
||
|
||
class CheckReleasesHaveHealthDataTest(TestCase, SnubaTestCase): | ||
backend = SessionsReleaseHealthBackend() | ||
|
||
def run_test(self, expected, projects, releases, start=None, end=None): | ||
if not start: | ||
start = datetime.now() - timedelta(days=1) | ||
if not end: | ||
end = datetime.now() | ||
assert ( | ||
check_releases_have_health_data( | ||
self.backend.check_releases_have_health_data( | ||
self.organization.id, | ||
[p.id for p in projects], | ||
[r.version for r in releases], | ||
|
@@ -1892,3 +1893,11 @@ def test(self): | |
self.run_test([release_1], [other_project], [release_1]) | ||
self.run_test([release_1, release_2], [other_project], [release_1, release_2]) | ||
self.run_test([release_1, release_2], [self.project, other_project], [release_1, release_2]) | ||
|
||
|
||
class CheckReleasesHaveHealthDataTestMetrics( | ||
ReleaseHealthMetricsTestCase, CheckReleasesHaveHealthDataTest | ||
): | ||
"""Repeat tests with metrics backend""" | ||
|
||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding here a
# type: ignore
is unnecessary imo.. I'd much rather not haveReleaseName
as a type and return astr
here sinceReleaseName
is just basically anstr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please
# type: ignore
comments should not be used liberally, and there must be a good reason to actually add themThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, #type: ignore should not be used liberally, it is necessary here to avoid error generated by
row.get()
:Regarding the point on ReleaseName I disagree though. Any way ReleaseName is used in more APIs.