ref(releasehealth): Implement check_has_health_data on metrics backend [INGEST-249] #28729

RaduW · 2021-09-21T15:36:44Z

Like in #28598, port check_has_health_data from sentry.snuba.sessions to metrics.

INGEST-249

untitaker · 2021-09-23T10:02:46Z

src/sentry/releasehealth/base.py

@@ -100,3 +108,16 @@ def get_release_adoption(
        """

        raise NotImplementedError()
+
+    def check_has_health_data(
+        self, projects_list: Sequence[Union[ProjectId, ProjectRelease]]


There are two problems with this type:

There's no relation between input and output.. type signature implies that one can pass in releases, and get out just projects. Or that one passes in projects and gets out releases (sometimes?)

Union at this position implies that you can freely mix ProjectId and ProjectRelease within the same sequence but the implementation doesn't handle that

You can make this generic over a T that is bounded by Union[ProjectId, ProjectRelease]. Then you have the guarantee that the same union variant is returned as is put into it.

T = TypeVar("ProjectOrRelease", bounds=Union[...]) def check_has_health_data(self, projects_list: Sequence[T]) -> Set[T]: ...

This will make the function generic over T: Union[...]. You should probably pick a better name for T though.

Thanks for the input, will do.

From what I understand making T=TypeVar("T", bounds=Union[ProjectId,ProjectRelease]) would not improve the semantics in any way, but making the definition T=TypeVar("T", ProjectId, ProjectRelease) would do exactly what you suggest.

Right, that may be better. However in any case it will be better, because when you call check_has_health_data you can declare T to be specifically ProjectId either way

untitaker · 2021-09-23T10:04:44Z

src/sentry/releasehealth/metrics.py

+        if len(projects_list) == 0:
+            return set()
+
+        includes_releases = type(projects_list[0]) == tuple


Suggested change

includes_releases = type(projects_list[0]) == tuple

includes_releases = isinstance(projects_list[0], tuple)

I believe this may make some type: ignore obsolete

untitaker · 2021-09-23T10:06:30Z

src/sentry/releasehealth/metrics.py

+            def extract_raw_info(
+                row: Mapping[str, Union[int, str]]
+            ) -> Union[ProjectId, ProjectRelease]:
+                return row.get("project_id")  # type: ignore


Suggested change

return row.get("project_id") # type: ignore

return row["project_id"] # type: ignore

either we're sure of our types or we should use Optional... there's no point in type-safety if everything is strictly typed and nulls still roam around freely IMO

untitaker · 2021-09-23T10:07:36Z

src/sentry/releasehealth/metrics.py

+        if includes_releases:
+            project_ids = [x[0] for x in projects_list]  # type: ignore
+        else:
+            project_ids = [x for x in projects_list]


Suggested change

project_ids = [x for x in projects_list]

project_ids = projects_list

of course :-), thanks

untitaker · 2021-09-23T10:15:29Z

src/sentry/releasehealth/sessions.py

-from sentry.snuba.sessions import _get_release_adoption, get_current_and_previous_crash_free_rates
+from sentry.snuba.sessions import (
+    _get_release_adoption,
+    check_has_health_data,


I suggest renaming this function to lead with underscore, you will find some more places where it's being called. One is inside of sentry.snuba.sessions itself.

You could argue it doesn't matter as sentry.snuba.sessions already implies we're using teh sessions backend, but I think we should call through the service as much as possible in case we really want to implement dual-read-and-compare.

src/sentry/releasehealth/metrics.py

ahmedetefy · 2021-09-24T13:39:29Z

src/sentry/releasehealth/metrics.py

+            where_clause.append(Condition(Column(release_column_name), Op.IN, releases_ids))
+            column_names = ["project_id", release_column_name]
+
+            # def extract_raw_info(row: Mapping[str, Union[int, str]]) -> ProjectOrRelease:


Why is there commented out code in this PR?

Fixed in next PR: #28823

ahmedetefy · 2021-09-24T13:43:36Z

tbh peppering the PR with # type: ignore seems like it defeats the purpose of using types in the first place

ahmedetefy · 2021-09-24T13:45:37Z

src/sentry/releasehealth/metrics.py

+        extract_raw_info = extract_raw_info_func(includes_releases)
+
+        query_cols = [Column(column_name) for column_name in column_names]
+        group_by_clause = query_cols


What is the point of defining group_by_clause here?

some people do it to signal intent (Query(select=query_cols, groupby=query_cols) looks like a copypaste error) but I wouldn't have done it either

ahmedetefy · 2021-09-24T13:52:37Z

Tbh, it feels strange to me seeing releasehealth as one word in a python code base, why is the module name not release_health
And I feel like its a misleading name too .. the abstraction layer is for sessions, no?
I would have just called it sessions_abstraction or sessions_v3

untitaker · 2021-09-24T13:58:45Z

@ahmedetefy let's resolve those questions outside of this PR

…d [INGEST-249] (#28729) check_has_health_data implemented for metrics

wip

9ae6dee

RaduW force-pushed the feat/check_has_health_data branch from 10f6d0a to 9ae6dee Compare September 21, 2021 15:41

vercel bot deployed to Preview – storybook September 21, 2021 15:41 View deployment

vercel bot deployed to Preview – sentry September 21, 2021 15:41 View deployment

wip

fbacbdf

vercel bot deployed to Preview – storybook September 21, 2021 15:57 View deployment

vercel bot deployed to Preview – sentry September 21, 2021 15:57 View deployment

merged master

4af9b96

vercel bot deployed to Preview – sentry September 22, 2021 10:59 View deployment

vercel bot deployed to Preview – storybook September 22, 2021 10:59 View deployment

check_has_health_data implemented

edfb48b

vercel bot deployed to Preview – sentry September 23, 2021 09:37 View deployment

vercel bot deployed to Preview – storybook September 23, 2021 09:37 View deployment

RaduW requested review from untitaker and jjbayer September 23, 2021 09:38

RaduW marked this pull request as ready for review September 23, 2021 09:38

RaduW requested a review from a team as a code owner September 23, 2021 09:38

Merge branch 'master' into feat/check_has_health_data

77bd804

vercel bot deployed to Preview – sentry September 23, 2021 10:11 View deployment

vercel bot deployed to Preview – storybook September 23, 2021 10:11 View deployment

untitaker requested changes Sep 23, 2021

View reviewed changes

untitaker removed the request for review from a team September 23, 2021 10:18

jjbayer reviewed Sep 23, 2021

View reviewed changes

src/sentry/releasehealth/metrics.py Outdated Show resolved Hide resolved

jjbayer approved these changes Sep 23, 2021

View reviewed changes

untitaker changed the title ~~ref(releasehealth): Implement check_has_health_data on metrics backend~~ ref(releasehealth): Implement check_has_health_data on metrics backend [INGEST-249] Sep 23, 2021

include review comments

b7b0f49

vercel bot deployed to Preview – sentry September 23, 2021 16:02 View deployment

vercel bot deployed to Preview – storybook September 23, 2021 16:02 View deployment

fix mypy typing errors

a636369

vercel bot deployed to Preview – sentry September 23, 2021 16:34 View deployment

vercel bot deployed to Preview – storybook September 24, 2021 07:40 View deployment

RaduW requested a review from untitaker September 24, 2021 07:40

remove change from next PR

cfb6a1b

vercel bot deployed to Preview – sentry September 24, 2021 08:27 View deployment

vercel bot deployed to Preview – storybook September 24, 2021 08:27 View deployment

untitaker approved these changes Sep 24, 2021

View reviewed changes

always dispatch through releasehealth service

4d9a65a

vercel bot deployed to Preview – sentry September 24, 2021 09:35 View deployment

vercel bot deployed to Preview – storybook September 24, 2021 09:35 View deployment

Merge branch 'master' into feat/check_has_health_data

d20ae72

vercel bot deployed to Preview – sentry September 24, 2021 09:39 View deployment

vercel bot deployed to Preview – storybook September 24, 2021 09:39 View deployment

RaduW enabled auto-merge (squash) September 24, 2021 09:39

untitaker approved these changes Sep 24, 2021

View reviewed changes

RaduW disabled auto-merge September 24, 2021 10:01

force CI

d5378f8

vercel bot deployed to Preview – sentry September 24, 2021 11:45 View deployment

vercel bot deployed to Preview – storybook September 24, 2021 11:45 View deployment

fix mock functions

91a4717

vercel bot deployed to Preview – sentry September 24, 2021 13:02 View deployment

vercel bot deployed to Preview – storybook September 24, 2021 13:02 View deployment

RaduW merged commit 3d787e2 into master Sep 24, 2021

RaduW deleted the feat/check_has_health_data branch September 24, 2021 13:26

ahmedetefy reviewed Sep 24, 2021

View reviewed changes

vuluongj20 pushed a commit that referenced this pull request Sep 30, 2021

ref(releasehealth): Implement check_has_health_data on metrics backen…

b462301

…d [INGEST-249] (#28729) check_has_health_data implemented for metrics

github-actions bot locked and limited conversation to collaborators Oct 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(releasehealth): Implement check_has_health_data on metrics backend [INGEST-249] #28729

ref(releasehealth): Implement check_has_health_data on metrics backend [INGEST-249] #28729

RaduW commented Sep 21, 2021

untitaker Sep 23, 2021

RaduW Sep 23, 2021 •

edited

Loading

untitaker Sep 23, 2021

untitaker Sep 23, 2021

untitaker Sep 23, 2021

RaduW Sep 23, 2021

untitaker Sep 23, 2021

RaduW Sep 23, 2021

untitaker Sep 23, 2021

ahmedetefy Sep 24, 2021

untitaker Sep 24, 2021

ahmedetefy commented Sep 24, 2021

ahmedetefy Sep 24, 2021

untitaker Sep 24, 2021

ahmedetefy commented Sep 24, 2021 •

edited

Loading

untitaker commented Sep 24, 2021

	includes_releases = type(projects_list[0]) == tuple
	includes_releases = isinstance(projects_list[0], tuple)

	return row.get("project_id") # type: ignore
	return row["project_id"] # type: ignore

	project_ids = [x for x in projects_list]
	project_ids = projects_list

ref(releasehealth): Implement check_has_health_data on metrics backend [INGEST-249] #28729

ref(releasehealth): Implement check_has_health_data on metrics backend [INGEST-249] #28729

Conversation

RaduW commented Sep 21, 2021

Choose a reason for hiding this comment

RaduW Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedetefy commented Sep 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedetefy commented Sep 24, 2021 • edited Loading

untitaker commented Sep 24, 2021

RaduW Sep 23, 2021 •

edited

Loading

ahmedetefy commented Sep 24, 2021 •

edited

Loading