feat(ddm): Add support for blocking metrics #63598

iambriccardo · 2024-01-22T16:05:00Z

This PR adds the ability to store blocked metrics in sentry options and allows to return of those blocked metrics in the metrics/meta endpoint.

A follow-up PR will be done in which the metrics configuration will be emitted for Relay. It was not done in this PR to not add additional complexity and surface for errors.

sentry-io · 2024-01-22T16:05:12Z

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: src/sentry/relay/config/init.py

Function	Unhandled Issue
`_get_project_config`	TypeError: 'Organization' object is not iterable ... `Event Count:` 41
`_get_project_config`	TypeError: 'int' object is not iterable sentry.ta... `Event Count:` 1

_{Did you find this useful? React with a 👍 or 👎}

codecov · 2024-01-22T16:32:10Z

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (6f3a2e4) 81.38% compared to head (a343e9d) 81.37%.
Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #63598      +/-   ##
==========================================
- Coverage   81.38%   81.37%   -0.01%     
==========================================
  Files        5221     5224       +3     
  Lines      231753   231924     +171     
  Branches    40066    40111      +45     
==========================================
+ Hits       188603   188734     +131     
- Misses      37344    37369      +25     
- Partials     5806     5821      +15

Files	Coverage Δ
src/sentry/api/paginator.py	`88.73% <100.00%> (ø)`
src/sentry/api/urls.py	`100.00% <ø> (ø)`
src/sentry/features/__init__.py	`100.00% <100.00%> (ø)`
src/sentry/new_migrations/monkey/__init__.py	`77.77% <100.00%> (ø)`
src/sentry/sentry_metrics/visibility/__init__.py	`100.00% <100.00%> (ø)`
src/sentry/sentry_metrics/visibility/errors.py	`100.00% <100.00%> (ø)`
src/sentry/snuba/metrics/utils.py	`96.80% <100.00%> (+0.10%)`	⬆️
src/sentry/testutils/cases.py	`85.67% <100.00%> (ø)`
...app/components/modals/inviteMembersModal/index.tsx	`92.42% <ø> (-0.23%)`	⬇️
.../settings/organizationMembers/inviteRequestRow.tsx	`100.00% <ø> (ø)`
... and 4 more

... and 7 files with indirect coverage changes

src/sentry/api/endpoints/organization_ddm.py

@@ -123,3 +129,32 @@
            )

        return Response(response, status=200)
+
+


src/sentry/api/endpoints/organization_metrics.py

iambriccardo · 2024-01-23T15:53:13Z

src/sentry/api/endpoints/organization_metrics.py

+    publish_status = {"GET": ApiPublishStatus.EXPERIMENTAL, "PATCH": ApiPublishStatus.EXPERIMENTAL}
+    owner = ApiOwner.TELEMETRY_EXPERIENCE
+
+    def get(self, request: Request, organization) -> Response:


Moved the metrics/meta endpoint also here, to prepare for a future refactor.

iambriccardo · 2024-01-23T15:54:03Z

src/sentry/api/urls.py

        OrganizationMetricsEndpoint.as_view(),
        name="sentry-api-0-organization-metrics-index",
    ),
+    re_path(
+        r"^(?P<organization_slug>[^/]+)/metrics/meta/$",


This is the previous endpoint, which is kept for backwards compatibility.

iambriccardo · 2024-01-23T15:54:55Z

src/sentry/sentry_metrics/visibility/metrics_blocking.py

+        try:
+            blocked_metrics_payload = json.loads(json_payload)
+        except ValueError:
+            if repair:


Repairing on write allows us to make sure we don't end up in a broken state of the system which requires a separate PR to unbreak.

I do understand that this is a real possibility since anyone else could theoretically write to this field, but I do not see a value in trying to "repair" it by deleting the key completely. Raising an exception and opening an issue on Sentry would be enough IMO.

I can remove this behavior, but if something is corrupted, we either delete it or figure out its content and make a PR to untangle it programmatically.

My main idea for doing so is that if a config of a user gets corrupted, they will now ingest all their blocked metrics and they don't have a way to block them again (they have to wait for our PR). With my system, they can immediately fix the problem with the downside being losing their previous settings. Still, it's a negligible problem since if the JSON is corrupted there likely won't be valuable data in the stored settings.

iambriccardo · 2024-01-23T15:55:43Z

src/sentry/sentry_metrics/visibility/metrics_blocking.py

+
+        return BlockedMetrics(metrics=blocked_metrics)._merge_blocked_metrics()
+
+    def _merge_blocked_metrics(self) -> "BlockedMetrics":


We run merging on both writing and reading, this assumes that options data can be overridden by someone else, so as long as the data is in the right format, we will merge.

We need merging because of the tags?

Nope, we need merging in general, since we don't want to return to the frontend duplicated data.

iambriccardo · 2024-01-23T15:56:29Z

src/sentry/snuba/metrics/datasource.py

-                type=parsed_mri.entity,
-                project_ids=project_ids,
+        blocked_for_projects = []
+        if (blocked := blocked_metrics.get(metric_mri)) is not None:


Too much manipulation going on here but if we have to merge blocked and non-blocked metrics we have no other ways.

iambriccardo · 2024-01-23T15:59:45Z

src/sentry/testutils/cases.py

@@ -2904,7 +2904,7 @@ def build_and_store_session(
        self.store_session(self.build_session(**kwargs))


-class OrganizationMetricMetaIntegrationTestCase(MetricsAPIBaseTestCase):
+class OrganizationMetricsIntegrationTestCase(MetricsAPIBaseTestCase):


driveby: Renamed for consistency.

iambriccardo · 2024-01-23T16:00:12Z

src/sentry/snuba/metrics/datasource.py


-    for metric_mri, project_ids in stored_mris.items():
+    stored_metrics = get_stored_metrics_of_projects(projects, use_case_id)


driveby: Removed for consistency.

obostjancic

Looks fine overall, left a couple of comments. Also, as mentioned before, I think we should try to keep refactors to a minimum during feature impl.

obostjancic · 2024-01-23T20:06:10Z

src/sentry/sentry_metrics/visibility/metrics_blocking.py

+
+@dataclass(frozen=True)
+class BlockedMetric:
+    metric_mri: str


nit:

Suggested change

metric_mri: str

mri: str

obostjancic · 2024-01-23T20:17:56Z

src/sentry/sentry_metrics/visibility/metrics_blocking.py

+        try:
+            blocked_metrics_payload = json.loads(json_payload)
+        except ValueError:
+            if repair:


I do understand that this is a real possibility since anyone else could theoretically write to this field, but I do not see a value in trying to "repair" it by deleting the key completely. Raising an exception and opening an issue on Sentry would be enough IMO.

obostjancic · 2024-01-23T20:18:57Z

src/sentry/sentry_metrics/visibility/metrics_blocking.py

+
+        return BlockedMetrics(metrics=blocked_metrics)._merge_blocked_metrics()
+
+    def _merge_blocked_metrics(self) -> "BlockedMetrics":


We need merging because of the tags?

obostjancic · 2024-01-23T20:24:00Z

src/sentry/snuba/metrics/datasource.py

@@ -1,5 +1,7 @@
 from __future__ import annotations

+from sentry.sentry_metrics.visibility import get_blocked_metrics


nit: do we plan on putting anything else in this module? If not, then i would call it blocking or metrics_blocking or would flatten it out by butting exports and errors in metrics_blocking.py

Yes we are, all the deletion logic will go there. It's called visibility exactly because it's a generic term that both have in common.

iambriccardo added 2 commits January 22, 2024 17:03

feat(ddm): Add support for blocking metrics

83833d1

Fix

973da32

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 22, 2024

iambriccardo changed the title ~~riccardo/feat/blocked metrics~~ feat(ddm): Add support for blocking metrics Jan 22, 2024

vercel bot deployed to Preview January 22, 2024 16:07 View deployment

Improved

bc565a9

vercel bot deployed to Preview January 23, 2024 10:04 View deployment

github-advanced-security bot found potential problems Jan 23, 2024

View reviewed changes

Fix

88ef032

vercel bot deployed to Preview January 23, 2024 13:49 View deployment

iambriccardo added 3 commits January 23, 2024 15:18

Improve

b15eea4

Improve

5d85758

Merge branch 'master' into riccardo/feat/blocked-metrics

bc63dd3

vercel bot deployed to Preview January 23, 2024 14:23 View deployment

Improving

f665bb7

vercel bot deployed to Preview January 23, 2024 14:42 View deployment

github-advanced-security bot found potential problems Jan 23, 2024

View reviewed changes

src/sentry/api/endpoints/organization_metrics.py Dismissed Show dismissed Hide dismissed

Improve code

946aae1

vercel bot deployed to Preview January 23, 2024 15:44 View deployment

Fix

d5abe91

iambriccardo commented Jan 23, 2024

View reviewed changes

vercel bot deployed to Preview January 23, 2024 15:55 View deployment

iambriccardo commented Jan 23, 2024

View reviewed changes

Fix endpoint name

a343e9d

iambriccardo requested a review from obostjancic January 23, 2024 15:58

iambriccardo marked this pull request as ready for review January 23, 2024 15:59

iambriccardo requested review from a team as code owners January 23, 2024 15:59

iambriccardo commented Jan 23, 2024

View reviewed changes

vercel bot deployed to Preview January 23, 2024 16:00 View deployment

obostjancic approved these changes Jan 23, 2024

View reviewed changes

obostjancic mentioned this pull request Jan 24, 2024

feat(metrics): Block metric tags with glob pattern getsentry/relay#2973

Merged

iambriccardo merged commit f201c69 into master Jan 24, 2024
50 of 51 checks passed

iambriccardo deleted the riccardo/feat/blocked-metrics branch January 24, 2024 08:49

snigdhas pushed a commit that referenced this pull request Jan 26, 2024

feat(ddm): Add support for blocking metrics (#63598)

f771681

github-actions bot locked and limited conversation to collaborators Feb 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ddm): Add support for blocking metrics #63598

feat(ddm): Add support for blocking metrics #63598

iambriccardo commented Jan 22, 2024 •

edited

Loading

sentry-io bot commented Jan 22, 2024

codecov bot commented Jan 22, 2024 •

edited

Loading

iambriccardo Jan 23, 2024

iambriccardo Jan 23, 2024

iambriccardo Jan 23, 2024

obostjancic Jan 23, 2024

iambriccardo Jan 24, 2024

iambriccardo Jan 23, 2024

obostjancic Jan 23, 2024

iambriccardo Jan 24, 2024

iambriccardo Jan 23, 2024

iambriccardo Jan 23, 2024

iambriccardo Jan 23, 2024

obostjancic left a comment

obostjancic Jan 23, 2024

obostjancic Jan 23, 2024

obostjancic Jan 23, 2024

obostjancic Jan 23, 2024

iambriccardo Jan 24, 2024


		return BlockedMetrics(metrics=blocked_metrics)._merge_blocked_metrics()

		def _merge_blocked_metrics(self) -> "BlockedMetrics":


		for metric_mri, project_ids in stored_mris.items():
		stored_metrics = get_stored_metrics_of_projects(projects, use_case_id)

		@@ -1,5 +1,7 @@
		from __future__ import annotations

		from sentry.sentry_metrics.visibility import get_blocked_metrics

feat(ddm): Add support for blocking metrics #63598

feat(ddm): Add support for blocking metrics #63598

Conversation

iambriccardo commented Jan 22, 2024 • edited Loading

sentry-io bot commented Jan 22, 2024

🔍 Existing Issues For Review

codecov bot commented Jan 22, 2024 • edited Loading

Codecov Report

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

obostjancic left a 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

iambriccardo commented Jan 22, 2024 •

edited

Loading

codecov bot commented Jan 22, 2024 •

edited

Loading