-
-
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
Changes from 12 commits
8984411
e120c09
a29ab0d
8f90afe
103b522
419adb7
1bab33b
b6f5ee3
c076d81
0e4314c
8edfe8b
20fd594
fa2b08c
a49272d
cf72ce3
2d0b585
59edfc4
94689e7
bde2262
732aa53
8c2ece0
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 |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from datetime import timedelta | ||
|
|
||
| from django.utils import timezone | ||
| from rest_framework.request import Request | ||
| from rest_framework.response import Response | ||
|
|
||
| from sentry import features | ||
| from sentry.api.api_owners import ApiOwner | ||
| from sentry.api.api_publish_status import ApiPublishStatus | ||
| from sentry.api.base import cell_silo_endpoint | ||
| from sentry.api.bases.organization import OrganizationEndpoint | ||
| from sentry.models.organization import Organization | ||
| from sentry.tasks.summaries.utils import ONE_DAY | ||
| from sentry.tasks.summaries.weekly_report_cache import read_project_metrics | ||
| from sentry.utils.dates import floor_to_utc_day, to_datetime | ||
|
|
||
| SATURDAY_ISOWEEKDAY = 6 | ||
|
|
||
| METRIC_KEY_MAP = { | ||
| "e": "totalErrors", | ||
| "t": "totalTransactions", | ||
| } | ||
|
|
||
|
|
||
| def _expand_metrics(abbreviated: dict[str, int] | None) -> dict[str, int] | None: | ||
| if abbreviated is None: | ||
| return None | ||
| return {METRIC_KEY_MAP[k]: v for k, v in abbreviated.items() if k in METRIC_KEY_MAP} | ||
|
|
||
|
|
||
| def _compute_pct_change( | ||
| current: dict[str, int] | None, previous: dict[str, int] | None | ||
| ) -> dict[str, float | None] | None: | ||
| if current is None or previous is None: | ||
| return None | ||
| change: dict[str, float | None] = {} | ||
| for abbrev_key, camel_key in METRIC_KEY_MAP.items(): | ||
| cur = current.get(abbrev_key, 0) | ||
| prev = previous.get(abbrev_key, 0) | ||
| if prev == 0: | ||
| change[camel_key] = None if cur > 0 else 0.0 | ||
| else: | ||
| change[camel_key] = round(((cur - prev) / prev) * 100, 2) | ||
| return change | ||
|
|
||
|
|
||
| @cell_silo_endpoint | ||
| class OrganizationWeeklyReportMetricsEndpoint(OrganizationEndpoint): | ||
| publish_status = { | ||
| "GET": ApiPublishStatus.PRIVATE, | ||
| } | ||
| owner = ApiOwner.NOTIFICATIONS | ||
|
|
||
| def get(self, request: Request, organization: Organization) -> Response: | ||
| if not features.has( | ||
| "organizations:weekly-report-metrics-api", | ||
| organization, | ||
| actor=request.user, | ||
| ): | ||
| return Response(status=404) | ||
|
|
||
| today = floor_to_utc_day(timezone.now()) | ||
| days_since_saturday = (today.isoweekday() - SATURDAY_ISOWEEKDAY) % 7 | ||
| last_saturday = today - timedelta(days=days_since_saturday) | ||
| current_timestamp = last_saturday.timestamp() | ||
| previous_timestamp = current_timestamp - (ONE_DAY * 7) | ||
|
|
||
| projects = self.get_projects(request, organization) | ||
| project_ids = [p.id for p in projects] | ||
| project_map = {p.id: p for p in projects} | ||
|
|
||
| start_iso = to_datetime(current_timestamp - ONE_DAY * 7).isoformat() | ||
| end_iso = to_datetime(current_timestamp).isoformat() | ||
|
|
||
| if not project_ids: | ||
| return Response( | ||
| { | ||
| "start": start_iso, | ||
| "end": end_iso, | ||
| "dataAvailable": False, | ||
| "projects": [], | ||
| } | ||
| ) | ||
|
|
||
| metrics_by_project = read_project_metrics( | ||
| org_id=organization.id, | ||
| project_ids=project_ids, | ||
| current_timestamp=current_timestamp, | ||
| previous_timestamp=previous_timestamp, | ||
| ) | ||
|
|
||
| if not metrics_by_project: | ||
| return Response( | ||
| { | ||
| "start": start_iso, | ||
| "end": end_iso, | ||
| "dataAvailable": False, | ||
| "projects": [], | ||
| } | ||
| ) | ||
|
|
||
| projects_response = [] | ||
| for project_id, data in metrics_by_project.items(): | ||
| project = project_map.get(project_id) | ||
| if not project: | ||
| continue | ||
| current = data["current"] | ||
| previous = data["previous"] | ||
| projects_response.append( | ||
| { | ||
| "id": str(project_id), | ||
| "slug": project.slug, | ||
| "currentWeek": _expand_metrics(current), | ||
| "previousWeek": _expand_metrics(previous), | ||
| "change": _compute_pct_change(current, previous), | ||
| } | ||
| ) | ||
|
|
||
| return Response( | ||
| { | ||
| "start": start_iso, | ||
| "end": end_iso, | ||
| "dataAvailable": True, | ||
| "projects": projects_response, | ||
| } | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from datetime import timedelta | ||
| from typing import Any | ||
|
|
||
| from django.conf import settings | ||
| from sentry_redis_tools.clients import RedisCluster, StrictRedis | ||
|
|
||
| from sentry.utils import json, redis | ||
|
|
||
| CACHE_TTL = timedelta(days=14) | ||
| KEY_PREFIX = "wr:proj_metrics" | ||
|
|
||
|
|
||
| def _make_cache_key(org_id: int, project_id: int, timestamp: float) -> str: | ||
| return f"{KEY_PREFIX}:{org_id}:{project_id}:{timestamp}" | ||
|
Member
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. including a timestamp in the cache key doesn't seem necessary, conceptually we should just be caching the latest value we computed for the 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, | ||
| timestamp: float, | ||
| project_metrics: dict[int, dict[str, int]], | ||
| ) -> None: | ||
| """ | ||
| Cache per-project weekly report metrics in Redis. | ||
|
|
||
| project_metrics maps project_id -> {"e": <total_errors>, "t": <total_transactions>} | ||
| """ | ||
| if not project_metrics: | ||
| return | ||
|
|
||
| client = _get_redis_client() | ||
| pipeline = client.pipeline() | ||
| ttl_seconds = int(CACHE_TTL.total_seconds()) | ||
|
|
||
| for project_id, metrics in project_metrics.items(): | ||
| key = _make_cache_key(org_id, project_id, timestamp) | ||
| pipeline.set(key, json.dumps(metrics), ex=ttl_seconds) | ||
|
|
||
| pipeline.execute() | ||
|
|
||
|
|
||
| def read_project_metrics( | ||
| org_id: int, | ||
| project_ids: list[int], | ||
| current_timestamp: float, | ||
| previous_timestamp: float, | ||
| ) -> dict[int, dict[str, Any]]: | ||
| """ | ||
| Read cached metrics for the current and previous week for each project. | ||
|
|
||
| Returns {project_id: {"current": {...} | None, "previous": {...} | None}} | ||
| """ | ||
| 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, current_timestamp)) | ||
| for project_id in project_ids: | ||
| pipeline.get(_make_cache_key(org_id, project_id, previous_timestamp)) | ||
|
|
||
| results = pipeline.execute() | ||
| n = len(project_ids) | ||
|
|
||
| result_map: dict[int, dict[str, Any]] = {} | ||
| for i, project_id in enumerate(project_ids): | ||
| current_raw = results[i] | ||
| previous_raw = results[n + i] | ||
|
|
||
| current = json.loads(current_raw) if current_raw else None | ||
| previous = json.loads(previous_raw) if previous_raw else None | ||
|
|
||
| if current is not None or previous is not None: | ||
| result_map[project_id] = { | ||
| "current": current, | ||
| "previous": previous, | ||
| } | ||
|
|
||
| return result_map | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ | |
| from taskbroker_client.retry import Retry | ||
| from taskbroker_client.worker.workerchild import ProcessingDeadlineExceeded | ||
|
|
||
| from sentry import analytics | ||
| from sentry import analytics, features | ||
| from sentry.analytics.events.weekly_report import WeeklyReportSent | ||
| from sentry.models.group import Group, GroupStatus | ||
| from sentry.models.grouphistory import GroupHistoryStatus | ||
|
|
@@ -37,6 +37,7 @@ | |
| OrganizationReportContextFactory, | ||
| ) | ||
| from sentry.tasks.summaries.utils import ONE_DAY, OrganizationReportContext | ||
| from sentry.tasks.summaries.weekly_report_cache import cache_project_metrics | ||
| from sentry.taskworker.namespaces import reports_tasks | ||
| from sentry.types.group import GroupSubStatus | ||
| from sentry.users.services.user_option import user_option_service | ||
|
|
@@ -162,6 +163,33 @@ def schedule_organizations( | |
| raise | ||
|
|
||
|
|
||
| def _cache_project_metrics( | ||
| ctx: OrganizationReportContext, organization_id: int, timestamp: float | ||
| ) -> None: | ||
| if not features.has("organizations:weekly-report-metrics-api", ctx.organization): | ||
| return | ||
|
|
||
| project_metrics: dict[int, dict[str, int]] = {} | ||
| for project_id, project_ctx in ctx.projects_context_map.items(): | ||
| if not project_ctx.check_if_project_is_empty(): | ||
| project_metrics[project_id] = { | ||
| "e": project_ctx.accepted_error_count, | ||
| "t": project_ctx.accepted_transaction_count, | ||
| } | ||
|
cursor[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| if not project_metrics: | ||
| return | ||
|
|
||
| with sentry_sdk.start_span(op="weekly_reports.cache_project_metrics"): | ||
| try: | ||
| cache_project_metrics(organization_id, timestamp, project_metrics) | ||
| except Exception: | ||
| logger.exception( | ||
| "weekly_reports.cache_project_metrics.failed", | ||
| extra={"organization_id": organization_id, "timestamp": timestamp}, | ||
| ) | ||
|
Contributor
Author
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. we might not need this logger?
Member
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. yeah think we can remove it |
||
|
|
||
|
|
||
| # This task is launched per-organization. | ||
| @instrumented_task( | ||
| name="sentry.tasks.summaries.weekly_reports.prepare_organization_report", | ||
|
|
@@ -217,6 +245,9 @@ def prepare_organization_report( | |
| lifecycle.record_halt(WeeklyReportHaltReason.EMPTY_REPORT) | ||
| return | ||
|
|
||
| # Cache per-project metrics for the API endpoint | ||
| _cache_project_metrics(ctx, organization_id, timestamp) | ||
|
Member
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. up to you, but maybe this should just call a top-level function in
Contributor
Author
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. agree -- moved this logic into |
||
|
|
||
| # Finally, deliver the reports | ||
| batch = OrganizationReportBatch(ctx, batch_id, dry_run, target_user, email_override) | ||
| with sentry_sdk.start_span(op="weekly_reports.deliver_reports"): | ||
|
|
||
|
Contributor
Author
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. only frontend change -- auto-added by santry bot |
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.
can we make this 10 days to prevent overlap of cache usage across 3 weeks?
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.
also, you can define this as an int in number of seconds, e.g. 10 * 24 * 60 * 60, so that you don't have to do that conversion below