ref(metrics): Release health service INGEST-376 #28598

jjbayer · 2021-09-15T14:59:07Z

Provide a common interface for release health queries, so we can easily switch between the "sessions" backend and the metrics backend.

Note: The service class in this PR only features a single release health related function. More functions will be ported in future PRs.

https://getsentry.atlassian.net/browse/INGEST-376

Workflow to port a function:

Select a function from sentry/snuba/sessions.py and add the same signature to sentry/releasehealth/base.py.
Add an implementation to sentry/releasehealth/sessions.py which calls the original function.
Add an implementation to sentry/releasehealth/metrics.py which retrieves the same information from the metrics dataset.
Prefix all existing calls to your function with releasehealth.
Go to tests/snuba/sessions/test_sessions.py and identify the test case corresponding to your function.
Inside the test case, prefix calls to your function with releasehealth.
Below that test case (let's call it Xx), add a class
```
class XxMetrics(ReleaseHealthMetricsTestCase, Xx):
    pass
```
This will repeat the test using the metrics backend.

Note: The metrics-based test will be skipped unless you have a snuba instance with metrics enabled:

Stop the snuba docker container started by devservices,

Start a local snuba with special config:

export SNUBA_SETTINGS=/path/to/settings_metrics.py
snuba bootstrap --force
snuba api

where settings_metrics.py looks like this:

from snuba.settings import *

ENABLE_DEV_FEATURES = True
DISABLED_DATASETS.remove("metrics")
SKIPPED_MIGRATION_GROUPS.remove("metrics")

src/sentry/snuba/sessions.py

untitaker

I think after basic review this is ready to merge and deploy. I'd rather deploy in small steps instead of later having to merge a large refactor branch

src/sentry/snuba/sessions.py

src/sentry/releasehealth/metrics.py

src/sentry/api/serializers/models/project.py

The existing API endpoint /tests/<dataset:dataset>/insert allows writing data to the default entity of each dataset. In the case of metrics, this means that only metrics_sets can be written. This PR introduces an additional endpoint /tests/entities/<entity:entity>/insert, which allows the client to define the target entity explicitly. This is required to run tests in getsentry/sentry#28598.

ahmedetefy · 2021-09-26T08:31:20Z

src/sentry/releasehealth/base.py

+from sentry.utils.services import Service
+
+
+class ReleaseHealthBackend(Service):  # type: ignore


Why is ReleaseHealthBackend inheriting from Service, Service has two functions that are validate and setup, that none of the ReleaseHealthBackend child classes define so I feel there is no point in this inheritance, and probably makes sense to just remove # type: ignore here
Also should the ReleaseHealthBackend class be an abstractclass?

Service is a special class which, in combination with LazyServiceWrapper, lets you expose the methods of the configured implementation as locals in the module. It's widely used in our code base, which is why we chose the same pattern for this abstraction. AFAIK nobody has gotten it to play nice with mypy yet, which is why we need all the #type: ignore comments.

I tried making this an abc.ABC initially, but ran into an issue I don't remember right now.

ahmedetefy · 2021-09-26T08:41:31Z

src/sentry/releasehealth/metrics.py

+
+
+def metric_id(org_id: int, name: str) -> int:
+    index = indexer.resolve(org_id, UseCase.TAG_KEY, name)  # type: ignore


@untitaker This is what I meant by places where # type: ignore can be avoided. Seems to me that indexer is of type StringIndexer , and resolve always returns int
typing the indexer imported should be enough to eliminate a few # type: ignore

indexer is a module. This is a problem of our lazy service abstraction which does too much magic for mypy to comprehend:

sentry/src/sentry/sentry_metrics/indexer/__init__.py

Line 12 in 2313490

backend.expose(locals())

We're certainly not going to type the indexer. That module is under active development by another team.

What I meant by typing the indexer is string_indexer: StringIndexer = indexer in this class given that this uses an instance of StringIndexer, no? and StringIndexer.resolve returns an int
Isn't that a potential way to eliminate this # type: ignore?

right but I believe we need a more generic solution to the service problem, I played around with this in hackweek and didn't come up with something satisfactory.

Just to be completely clear on this, I do believe some cases do require a # type: ignore but what I am advocating here is that we investigate prior to falling back on this
Imo type casting the instance of indexer imported here is a satisfactory alternative to ignoring the type

I understand, but we have investigated solutions to give service methods the correct types, I just don't agree with your suggestion as I don't think the time is well-spent in coming up with usage-specific workarounds (per usage of indexer)

ahmedetefy · 2021-09-26T08:53:57Z

src/sentry/releasehealth/metrics.py

+    return str_value  # type: ignore
+
+
+class MetricsReleaseHealthBackend(ReleaseHealthBackend):


Nit: naming this just MetricsBackend should be enough given that it inherits from ReleaseHealthBackend so probably not necessary to include ReleaseHealth in every child class

ahmedetefy · 2021-09-26T08:54:51Z

src/sentry/releasehealth/metrics.py

+        previous_end: datetime,
+        rollup: int,
+        org_id: Optional[int] = None,
+    ) -> ReleaseHealthBackend.CurrentAndPreviousCrashFreeRates:


Why are doing this kind of type inheritance ? I would just move the types outside of the class.. Surely they will be used in other areas of the code that use this function

This has been fixed in master.

ahmedetefy · 2021-09-26T08:55:05Z

src/sentry/releasehealth/metrics.py

+        if org_id is None:
+            org_id = self._get_org_id(project_ids)
+
+        projects_crash_free_rate_dict: ReleaseHealthBackend.CurrentAndPreviousCrashFreeRates = {


Same here as above comment

ahmedetefy · 2021-09-26T08:56:38Z

src/sentry/releasehealth/metrics.py

+
+        count_query = Query(
+            dataset=Dataset.Metrics.value,
+            match=Entity("metrics_counters"),


Would use an enum for metrics_counter.. this is error prone
instead
EntityKey.METRICS_COUNTER.value

ahmedetefy · 2021-09-26T09:01:42Z

src/sentry/releasehealth/sessions.py

+        rollup: int,
+        org_id: Optional[int] = None,
+    ) -> ReleaseHealthBackend.CurrentAndPreviousCrashFreeRates:
+        return get_current_and_previous_crash_free_rates(  # type: ignore


Defining a type for get_current_and_previous_crash_free_rates will prevent us from needing to add # type: ignore

ahmedetefy · 2021-09-26T09:03:32Z

tests/snuba/sessions/test_sessions.py

@@ -1779,6 +1789,10 @@ def test_get_current_and_previous_crash_free_rates_with_zero_sessions(self):
        }


+class GetCrashFreeRateTestCaseMetrics(ReleaseHealthMetricsTestCase, GetCrashFreeRateTestCase):
+    """Repeat tests with metrics backend"""
+


Please Add a ToDo comment

Deriving from both ReleaseHealthMetricsTestCase and GetCrashFreeRateTestCase already repeats the tests, so there is nothing to do here. But I agree that that should be clearer from reading the code.

jjbayer added 6 commits September 15, 2021 16:48

wip

e2bb02f

release health service skeleton

3874555

wip

c221322

wip

807c52a

wip

a2bb4ea

ref: First function ported to metrics

2e07d06

jjbayer requested review from untitaker and RaduW September 15, 2021 14:59

fix: Revert leftover code

d8ac922

vercel bot deployed to Preview – storybook September 16, 2021 07:52 View deployment

vercel bot deployed to Preview – sentry September 16, 2021 07:52 View deployment

jjbayer commented Sep 16, 2021

View reviewed changes

src/sentry/snuba/sessions.py Outdated Show resolved Hide resolved

untitaker requested changes Sep 16, 2021

View reviewed changes

src/sentry/snuba/sessions.py Outdated Show resolved Hide resolved

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

src/sentry/api/serializers/models/project.py Outdated Show resolved Hide resolved

jjbayer added 4 commits September 16, 2021 16:31

test: Repeat GetCrashFreeRateTestCase with metrics

7194379

ref: Put code back in snuba/sessions.py

351c30a

fix: Make org_id optional

7cb01e9

test: Drop metrics dataset before test

dbffecf

vercel bot deployed to Preview – storybook September 16, 2021 15:28 View deployment

vercel bot deployed to Preview – sentry September 16, 2021 15:28 View deployment

jjbayer added 2 commits September 17, 2021 11:57

fix: typing

4894ef7

fix: Only run metrics tests if snuba has them

8494337

vercel bot deployed to Preview – storybook September 17, 2021 12:48 View deployment

vercel bot deployed to Preview – sentry September 17, 2021 12:48 View deployment

jjbayer mentioned this pull request Sep 17, 2021

feat(web): Entity-specific insertion endpoint for tests getsentry/snuba#2101

Merged

fix: tests

5bd0501

vercel bot deployed to Preview – sentry September 17, 2021 13:34 View deployment

vercel bot deployed to Preview – storybook September 17, 2021 13:34 View deployment

jjbayer marked this pull request as ready for review September 17, 2021 13:53

jjbayer requested a review from a team as a code owner September 17, 2021 13:53

jjbayer requested a review from untitaker September 17, 2021 13:53

untitaker approved these changes Sep 20, 2021

View reviewed changes

jjbayer merged commit 31e6dd6 into master Sep 20, 2021

jjbayer deleted the ref/release-health-service branch September 20, 2021 08:49

untitaker mentioned this pull request Sep 21, 2021

ref(releasehealth): Implement get_release_adoption with metrics [INGEST-249] #28725

Merged

RaduW mentioned this pull request Sep 21, 2021

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

Merged

ahmedetefy reviewed Sep 26, 2021

View reviewed changes

This was referenced Oct 7, 2021

ref(releasehealth): Implement get_project_sessions_count on metrics backend [INGEST-249] #29148

Merged

ref(releasehealth): Implement get_num_sessions_per_project on metrics backend [INGEST-249] #29208

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref(metrics): Release health service INGEST-376 #28598

ref(metrics): Release health service INGEST-376 #28598

jjbayer commented Sep 15, 2021 •

edited

Loading

untitaker left a comment •

edited

Loading

ahmedetefy Sep 26, 2021

jjbayer Sep 27, 2021 •

edited

Loading

ahmedetefy Sep 26, 2021

untitaker Sep 27, 2021 •

edited

Loading

ahmedetefy Sep 27, 2021 •

edited

Loading

untitaker Sep 27, 2021

ahmedetefy Sep 27, 2021

untitaker Sep 27, 2021

ahmedetefy Sep 26, 2021

ahmedetefy Sep 26, 2021

untitaker Sep 27, 2021

ahmedetefy Sep 26, 2021

ahmedetefy Sep 26, 2021

ahmedetefy Sep 26, 2021

ahmedetefy Sep 26, 2021

jjbayer Sep 27, 2021

		from sentry.utils.services import Service


		class ReleaseHealthBackend(Service): # type: ignore



		def metric_id(org_id: int, name: str) -> int:
		index = indexer.resolve(org_id, UseCase.TAG_KEY, name) # type: ignore

		return str_value # type: ignore


		class MetricsReleaseHealthBackend(ReleaseHealthBackend):

ref(metrics): Release health service INGEST-376 #28598

ref(metrics): Release health service INGEST-376 #28598

Conversation

jjbayer commented Sep 15, 2021 • edited Loading

Workflow to port a function:

untitaker left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untitaker Sep 27, 2021 • edited Loading

Choose a reason for hiding this comment

ahmedetefy Sep 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjbayer commented Sep 15, 2021 •

edited

Loading

untitaker left a comment •

edited

Loading

jjbayer Sep 27, 2021 •

edited

Loading

untitaker Sep 27, 2021 •

edited

Loading

ahmedetefy Sep 27, 2021 •

edited

Loading