Skip to content

fix(slack): Use only first chart when unfurling multi-aggregate Explore URLs#113303

Merged
DominikB2014 merged 5 commits intomasterfrom
dominikbuszowiecki/dain-1551-multi-aggregate-queries-show-second-chart-instead-of-first
Apr 17, 2026
Merged

fix(slack): Use only first chart when unfurling multi-aggregate Explore URLs#113303
DominikB2014 merged 5 commits intomasterfrom
dominikbuszowiecki/dain-1551-multi-aggregate-queries-show-second-chart-instead-of-first

Conversation

@DominikB2014
Copy link
Copy Markdown
Contributor

@DominikB2014 DominikB2014 commented Apr 17, 2026

Url unfurls currently only supports one chart, this fixes a bug when multiple charts are present, we display the last chart instead of the first.

Ai comment
Explore URLs can contain multiple aggregateField entries, each of which maps to a separate chart in the Explore UI (per Visualize.fromJSON in static/app/views/explore/contexts/pageParamsContext/visualizes.tsx). The Slack unfurl was merging every yAxes entry into one events-timeseries request and inheriting a chartType from any later entry that defined one, so the rendered chart mixed series from all charts and could adopt the second chart's style (e.g. area) instead of the first chart's default.

Example URL from the issue — three aggregateField entries (empty groupBy, avg(span.duration), then count(span.duration) with chartType: 2): the unfurl was sending both yAxes to the API and rendering as area, when it should show just the first chart (avg(span.duration) as a line).

Take yAxes and chartType from only the first aggregateField that has yAxes so the unfurl matches the top-most chart on the Explore page. Group-bys still accumulate since they apply to the shared query rather than to a specific chart. Applied the same change to the metric JSON's nested aggregateFields.

Fixes DAIN-1551

…re URLs

Explore URLs can contain multiple aggregateField entries, each of which is
a separate chart (per Explore's Visualize.fromJSON). The unfurl was merging
every yAxes entry into one events-timeseries request and inheriting a
chartType from any later entry that defined one, so the rendered chart
mixed series from all charts and could adopt the second chart's style
(e.g. area) instead of the first chart's default.

Take yAxes and chartType from only the first aggregateField that has yAxes
so the unfurl renders the first chart, matching what the Explore page
shows at the top. Group-bys still accumulate since they apply to the
shared query rather than to a specific chart. Apply the same change to
the metric JSON's nested aggregateFields.

Fixes DAIN-1551
@linear-code
Copy link
Copy Markdown

linear-code Bot commented Apr 17, 2026

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 17, 2026
Comment thread src/sentry/integrations/slack/unfurl/explore.py Outdated
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 72d8eb7. Configure here.

Comment thread src/sentry/integrations/slack/unfurl/explore.py Outdated
@DominikB2014 DominikB2014 marked this pull request as ready for review April 17, 2026 16:16
@DominikB2014 DominikB2014 requested review from a team as code owners April 17, 2026 16:16
@DominikB2014 DominikB2014 marked this pull request as draft April 17, 2026 16:20
Comment thread src/sentry/integrations/slack/unfurl/explore.py Outdated
The bug lives in map_explore_query_args, so drop the mocked unfurl
execution and assert only on the link-matched args.
Switch to the real URL from DAIN-1551 (count first with chartType=2,
avg second). Pre-fix behavior: both yAxes merged into the API request,
which is why avg was visible in Slack alongside count's area chart.
A prior refactor narrowed the try/except around json.loads so that
subsequent parsed.get() / "in" parsed usage ran outside the handler.
json.loads can return non-dict values (int, list, string, null) which
then raise TypeError or AttributeError and crash the arg mapper.

Move the usage back inside the try and also catch AttributeError, so
non-dict JSON is silently skipped (falling back to the dataset default
yAxis) and add a regression test.
@DominikB2014 DominikB2014 marked this pull request as ready for review April 17, 2026 16:53
@DominikB2014 DominikB2014 merged commit 16a7c2d into master Apr 17, 2026
56 checks passed
@DominikB2014 DominikB2014 deleted the dominikbuszowiecki/dain-1551-multi-aggregate-queries-show-second-chart-instead-of-first branch April 17, 2026 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants