feat(weekly-report): Render top spans p95 chart - #119768
Conversation
66924bf to
a454bb6
Compare
| size={"width": 600, "height": 200}, | ||
| ) | ||
| except Exception: | ||
| logger.exception("weekly_report.spans_chart.generation_failed") |
There was a problem hiding this comment.
i think having this logger is important, esp when we first start rolling this out
| return None | ||
|
|
||
| cache_key = frozenset(span["name"] for span in table) | ||
| if spans_chart_cache is not None and cache_key in spans_chart_cache: |
There was a problem hiding this comment.
made linear ticket to add metrics to this "cache"
| chart_url = _top_spans_chart_url(table, ctx, spans_chart_cache) | ||
|
|
||
| return { | ||
| "total_spans_count": user_total_spans_count, | ||
| "top_spans_table": table, | ||
| "spans_chart_url": chart_url, |
There was a problem hiding this comment.
Top spans chart can include other projects' metrics
The new spans chart uses org-wide timeseries keyed only by span name after filtering the table by the user's projects, so p95 series can mix in latency data from projects the recipient cannot access when span names collide. Scope chart series generation to the recipient's projects (or project-scoped timeseries) before embedding the image.
Evidence
top_spans()only keeps spans whose assigned project is inuser_project_ids, then calls_top_spans_chart_url(table, ctx, spans_chart_cache)with that filtered table._top_spans_chart_urlbuilds series fromctx.top_spans_timeseries[span["name"]]with no further project filter.organization_top_spans_timeseries()queries all org transaction projects and groups only byspan.name, so the series is org-aggregated.- The resulting chart URL is embedded in the recipient email, so users can observe p95 values influenced by projects outside their ownership when names overlap.
Identified by Warden · security-review, wrdn-authz, wrdn-data-exfil · PA9-LCB
| logger.exception("weekly_report.spans_chart.generation_failed") | ||
|
|
||
| if spans_chart_cache is not None: | ||
| spans_chart_cache[cache_key] = chart_url |
There was a problem hiding this comment.
do we have to gate the caching based on dry runs as well? might be worth having a centralized path for that
There was a problem hiding this comment.
spans_chart_cache is an in-memory dict, so it won't persist across runs, so no need to gate behind dry_run flag
| </table> | ||
| {% endif %} | ||
|
|
||
| {% if notification_settings_link and user_project_count > 5 %} |
There was a problem hiding this comment.
this has only been around for a couple weeks right? i'd be down to show it for a bit longer unless u have a specific reason for removing it
There was a problem hiding this comment.
yea it's been in the past 2 reports. maybe we keep it for another week? --> i'll make a linear ticket so we remember to remove it
|
|
||
| chart_url = None | ||
| try: | ||
| chart_url = charts.generate_chart( |
There was a problem hiding this comment.
from the image in the PR description it seems like this might not have a unit for the time on the y axis? is there a way to add that?
There was a problem hiding this comment.
i looked into adding the units for the y-axis. i think it involves changing chartcuterie code (so outside scope). probably would be a useful feature. linear ticket
| <h4 style="margin-bottom: 0;">p95 for Top Spans</h4> | ||
| {% if spans_chart_url %} | ||
| <div style="margin-bottom: 16px;"> | ||
| <img src="{{ spans_chart_url }}" width="600" height="200" alt="Top spans p95 chart" style="max-width: 100%;" /> |
There was a problem hiding this comment.
did you check how this looks on mobile by any chance?
There was a problem hiding this comment.
will add screenshot once my devenv works :(
| <a href="{% org_url organization '/explore/traces/' %}" | ||
| style="font-size: 12px; margin-bottom: 16px; display: block; font-weight: bold;">View All Spans</a> | ||
|
|
||
| <h4 style="margin-bottom: 0;">p95 for Top Spans</h4> |
There was a problem hiding this comment.
same as sentry bot comment, should this header be inside the {% if spans_chart_url %}? bc we don't want to display it if we don't show the actual chart itself
| for span in ctx.top_spans | ||
| for i, span in enumerate(ctx.top_spans) | ||
| ] | ||
| if charts.is_enabled() and ctx.top_spans: |
There was a problem hiding this comment.
i wonder if the debug logic should just call the generate chart helper in the main code directly so that we don't have to worry abt keeping this logic in sync
|
|
||
| ctx = OrganizationReportContext(self.timestamp, ONE_DAY * 7, self.organization) | ||
| user_project_ownership(ctx) | ||
| ctx.top_spans = [{"name": "/api/users", "p95": 120.0, "sum": 50000.0}] |
There was a problem hiding this comment.
does spans_count_by_project have to be set in this test?
There was a problem hiding this comment.
spans_count_by_project needs to be set for both the test_spans_chart_url_in_template_context and test_spans_chart_url_cached_across_users b/c the functions exit early if count == 0
There was a problem hiding this comment.
won't this one also exit early, or does it exit early bc of missing FF first
There was a problem hiding this comment.
yea exits early b/c of missing FF
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5885b2b. Configure here.
| <img src="{{ spans_chart_url }}" width="600" height="200" alt="Top spans p95 chart" style="max-width: 100%;" /> | ||
| </div> | ||
| {% endif %} | ||
|
|
There was a problem hiding this comment.
Missing table heading without chart
Medium Severity
The previous Top Spans heading was removed and replaced by p95 for Top Spans, which only renders when spans_chart_url is set. If chart rendering is disabled or fails, top_spans_table still renders with color swatches but no section heading, so the table loses its label in a common fallback path.
Reviewed by Cursor Bugbot for commit 5885b2b. Configure here.
Add Chartcuterie-based line chart rendering for top-5 spans p95 timeseries in the weekly report email. The chart is filtered per user's project access and cached by the distinct set of visible span names to avoid redundant renders across users with identical project membership. Gated behind the organizations:weekly-report-spans-chart feature flag. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Bypass the feature flag in the debug view by generating the chart inline when Chartcuterie is enabled, matching the pattern used by other debug view overrides. Move chart section after resolved issues. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
The top_spans() helper gates on user_total_spans_count > 0, which requires spans_count_by_project to be populated. The chart tests were missing this setup.
5885b2b to
cf7a3da
Compare
| for span in ctx.top_spans | ||
| for i, span in enumerate(ctx.top_spans) | ||
| ] | ||
| chart_url = _top_spans_chart_url(context["top_spans_table"], ctx, None) |
There was a problem hiding this comment.
Span chart timeseries includes unfiltered org-wide project data
_top_spans_chart_url generates charts using ctx.top_spans_timeseries which is aggregated across all organization projects. While the calling top_spans() filters the visible span table by user_project_ids, the timeseries data fed into Chartcuterie is not scoped to user-accessible projects, so a user's chart can include performance contributions from inaccessible projects.
Evidence
organization_top_spans_timeseriesinutils.py:838queries Snuba with_get_transaction_projects(ctx)(all org projects with transactions) and groups byraw_groupby=["span.name"]only.- The resulting
ctx.top_spans_timeseries[span_name]contains p95 timeseries aggregated across every org project, with no per-project dimension. - In
render_template_context,top_spans()filters the visible span table byuser_project_ids, but_top_spans_chart_url(weekly_reports.py:628) looks upctx.top_spans_timeseries.get(span["name"], {})for each visible span without further project filtering. - This means a user's chart image is generated using performance data from projects they cannot access.
Identified by Warden · wrdn-data-exfil · 47S-5GT
**Summary** - Render a multi-series line chart via Chartcuterie (`SLACK_DISCOVER_TOP5_PERIOD_LINE`) and embed as an `<img>` tag in the weekly report email - Cache chart URLs by the distinct set of visible span names to avoid redundant renders across users with identical project membership **Rendering** (`weekly_reports.py`): - `top_spans_chart_url()` helper filters spans by user project access, checks/populates a `frozenset`-keyed cache, and calls Chartcuterie - Chart size: 600x200px **Template** (`body.html`): - New section after key performance issues, gated on `spans_chart_url` and `not enhanced_privacy` Website view: <img width="1300" height="1172" alt="image" src="https://github.com/user-attachments/assets/35e32563-8d31-47a6-980b-fc2a813110af" /> Mobile view: <img width="898" height="810" alt="image" src="https://github.com/user-attachments/assets/071e882e-c122-4aff-9ac0-7c658859fb55" /> ## Debug preview 1. **Build the Chartcuterie config:** ```bash pnpm build-chartcuterie-config ``` 2. **Start the Chartcuterie Docker container:** ```bash docker run --rm -d -p 7901:8000 --platform linux/amd64 --name chartcuterie \ -v "$(pwd)/config/chartcuterie/config.js:/etc/chartcuterie/config.js" \ us-central1-docker.pkg.dev/sentryio/chartcuterie/image:latest \ node ./lib/index.js server -c /etc/chartcuterie/config.js ``` 3. **Enable chart rendering** — add to `~/.sentry/sentry.conf.py`: ```python SENTRY_OPTIONS["chart-rendering.enabled"] = True ``` 4. **Start the dev server** and visit `http://localhost:8000/debug/mail/reports/body/` --------- Co-authored-by: Claude Opus 4 <noreply@anthropic.com> Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>


Summary
SLACK_DISCOVER_TOP5_PERIOD_LINE) and embed as an<img>tag in the weekly report emailRendering (
weekly_reports.py):top_spans_chart_url()helper filters spans by user project access, checks/populates afrozenset-keyed cache, and calls ChartcuterieTemplate (
body.html):spans_chart_urlandnot enhanced_privacyWebsite view:

Mobile view:

Debug preview
Build the Chartcuterie config:
Start the Chartcuterie Docker container:
docker run --rm -d -p 7901:8000 --platform linux/amd64 --name chartcuterie \ -v "$(pwd)/config/chartcuterie/config.js:/etc/chartcuterie/config.js" \ us-central1-docker.pkg.dev/sentryio/chartcuterie/image:latest \ node ./lib/index.js server -c /etc/chartcuterie/config.jsEnable chart rendering — add to
~/.sentry/sentry.conf.py:Start the dev server and visit
http://localhost:8000/debug/mail/reports/body/