-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(reports): Cache per-project weekly report metrics #116739
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
Merged
+127
−0
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
8984411
update weekly report dev environment
amy-chen23 e120c09
feature flag for cache api endpoint
amy-chen23 a29ab0d
cache logic for weekly report. cache is populated when the weekly rep…
amy-chen23 8f90afe
:hammer_and_wrench: Sync API Urls to TypeScript
getsantry[bot] 103b522
gate cache layer behind feature flag
amy-chen23 419adb7
moving imports up
amy-chen23 1bab33b
Merge branch 'master' into amyc/cache-weekly-report-metrics
amy-chen23 b6f5ee3
removing frontend changes
amy-chen23 c076d81
removing frontend change (for real this time)
amy-chen23 0e4314c
:hammer_and_wrench: Sync API Urls to TypeScript
getsantry[bot] 8edfe8b
fixing cache miss issues with SATURDAY send time
amy-chen23 20fd594
imported correct redis cluster. also added -> None to all test method…
amy-chen23 fa2b08c
removed API endpoint logic
amy-chen23 a49272d
removing file diff
amy-chen23 cf72ce3
adding cache miss metrics
amy-chen23 2d0b585
adding math.floor for saturday (so less cache misses)
amy-chen23 59edfc4
removed timestamp from cache key
amy-chen23 94689e7
no caching during dry run
amy-chen23 bde2262
added feature flag. moved metric extraction to weekly_reports.py so t…
amy-chen23 732aa53
removing feature flag and wrapping caching in a try-catch
amy-chen23 8c2ece0
remove extra conversion of cache ttl
amy-chen23 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from typing import Any | ||
|
|
||
| from django.conf import settings | ||
| from sentry_redis_tools.clients import RedisCluster, StrictRedis | ||
|
|
||
| from sentry.utils import json, metrics, redis | ||
|
|
||
| CACHE_TTL_SEC = 10 * 24 * 60 * 60 # 10 days | ||
| KEY_PREFIX = "wr:proj_metrics" | ||
|
|
||
|
|
||
| def _make_cache_key(org_id: int, project_id: int) -> str: | ||
| return f"{KEY_PREFIX}:{org_id}:{project_id}" | ||
|
|
||
|
|
||
| def _get_redis_client() -> RedisCluster[str] | StrictRedis[str]: | ||
| return redis.redis_clusters.get(settings.SENTRY_WEEKLY_REPORTS_REDIS_CLUSTER) | ||
|
|
||
|
|
||
| def cache_project_metrics( | ||
| org_id: int, | ||
| project_metrics: dict[int, dict[str, int]], | ||
| ) -> None: | ||
| client = _get_redis_client() | ||
| pipeline = client.pipeline() | ||
|
|
||
| for project_id, values in project_metrics.items(): | ||
| key = _make_cache_key(org_id, project_id) | ||
| pipeline.set(key, json.dumps(values), ex=CACHE_TTL_SEC) | ||
|
|
||
| pipeline.execute() | ||
|
|
||
|
|
||
| def read_project_metrics( | ||
| org_id: int, | ||
| project_ids: list[int], | ||
| ) -> dict[int, dict[str, Any]]: | ||
| if not project_ids: | ||
| return {} | ||
|
|
||
| client = _get_redis_client() | ||
| pipeline = client.pipeline() | ||
|
|
||
| for project_id in project_ids: | ||
| pipeline.get(_make_cache_key(org_id, project_id)) | ||
|
|
||
| results = pipeline.execute() | ||
|
|
||
| result_map: dict[int, dict[str, Any]] = {} | ||
| for i, project_id in enumerate(project_ids): | ||
| raw = results[i] | ||
| if raw is None: | ||
| metrics.incr("weekly_report.cache.miss") | ||
| else: | ||
| result_map[project_id] = json.loads(raw) | ||
|
|
||
| return result_map | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| from sentry.tasks.summaries.weekly_report_cache import ( | ||
| _make_cache_key, | ||
| cache_project_metrics, | ||
| read_project_metrics, | ||
| ) | ||
| from sentry.testutils.cases import TestCase | ||
|
|
||
|
|
||
| class WeeklyReportCacheTest(TestCase): | ||
| def test_make_cache_key(self) -> None: | ||
| key = _make_cache_key(org_id=1, project_id=2) | ||
| assert key == "wr:proj_metrics:1:2" | ||
|
|
||
| def test_write_and_read(self) -> None: | ||
| org_id = self.organization.id | ||
| project = self.create_project(organization=self.organization) | ||
|
|
||
| cache_project_metrics(org_id, {project.id: {"e": 500, "t": 3000}}) | ||
|
|
||
| result = read_project_metrics(org_id=org_id, project_ids=[project.id]) | ||
|
|
||
| assert result[project.id] == {"e": 500, "t": 3000} | ||
|
|
||
| def test_read_empty_cache(self) -> None: | ||
| result = read_project_metrics(org_id=self.organization.id, project_ids=[999]) | ||
|
|
||
| assert result == {} | ||
|
|
||
| def test_read_empty_project_ids(self) -> None: | ||
| result = read_project_metrics(org_id=self.organization.id, project_ids=[]) | ||
|
|
||
| assert result == {} | ||
|
|
||
| def test_write_empty_metrics_is_noop(self) -> None: | ||
| cache_project_metrics(self.organization.id, {}) | ||
|
|
||
| def test_multiple_projects(self) -> None: | ||
| org_id = self.organization.id | ||
| p1 = self.create_project(organization=self.organization) | ||
| p2 = self.create_project(organization=self.organization) | ||
|
|
||
| cache_project_metrics( | ||
| org_id, | ||
| { | ||
| p1.id: {"e": 100, "t": 200}, | ||
| p2.id: {"e": 300, "t": 400}, | ||
| }, | ||
| ) | ||
|
|
||
| result = read_project_metrics(org_id=org_id, project_ids=[p1.id, p2.id]) | ||
|
|
||
| assert result[p1.id] == {"e": 100, "t": 200} | ||
| assert result[p2.id] == {"e": 300, "t": 400} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
should we record cache hits as well? another way you could do this would be to have a single metric called
weekly_report.cache_readand then a tag on the metric is set to either "miss" or "hit" based on the result, and then DD allows you to break down those timeseriesThere 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.
this can be part of your next PR btw, doesn't have to block this one