feat(logs): Wire Seer AI visualization params into logs explore#115585
feat(logs): Wire Seer AI visualization params into logs explore#115585isaacwang-sentry wants to merge 6 commits into
Conversation
github-actions
Bot
added
the
Scope: Frontend
isaacwang-sentry
force-pushed
the
codex/logs-ai-visualization-wiring
branch
from
ecd9e6b to
c6d8bea
Compare
📊 Type Coverage Diff✅ No new type safety issues introduced. Coverage: 93.57% |
isaacwang-sentry
force-pushed
the
codex/logs-ai-visualization-wiring
branch
from
c6d8bea to
a1d6489
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Sorry for the delay - this is new code for me and I wanted to process it for a bit. I think there are some bugs (shoutout Cursor for helping me validate suspicions). And the duplication scares me.
I'm off through EOD Monday but sent this over to the Explore team to see if someone else has more context.
Exciting stuff, really cool to see the visualizations piping through! 🔥
There was a problem hiding this comment.
[Refactor] A lot of the areas of code being extracted here are existing duplicates of other explore/*/*TabSeerComboBox.tsx files. getLogsSeerLocationQuery in particular matches that. The duplication was already a risk of drift-y bugs (e.g. #116050). I think this PR's refactoring just one component makes that drift more severe and risky.
Instead of continuing to drift, could you please deduplicate the roughly-identical logic from all five places? E.g. extracting a util function like getSeerLocationQuery / getSeerComboBoxLocationQuery?
| end: selection.datetime.end?.toString() ?? null, | ||
| statsPeriod: selection.datetime.period, | ||
| utc: selection.datetime.utc?.toString() ?? null, | ||
| }; |
There was a problem hiding this comment.
[Bug] I'm not super confident in this, but I think this doesn't appropriately clear existing query param cursor keys? After Seer applies a query the user will land mid-paginated results from their old state. Other query-update code paths in this file clear the cursor. E.g., updateLocationWithLogSortBys: it either sets or deletes location.query[LOGS_CURSOR_KEY].
I think this needs the same treatment here, and probably also LOGS_AGGREGATE_CURSOR_KEY when switching to aggregate mode.
| ? existingVisualizes | ||
| : defaultVisualizes(true).map(visualize => visualize.serialize()); | ||
|
|
||
| return [...groupBys.map(groupBy => ({groupBy})), ...visualizes]; |
There was a problem hiding this comment.
[Bug] Is it intentional that you wanted all group-bys before visualizes?
For context, the old applySeerSearchQuery had a groupByAfterVisualizes pass that detected whether the current list had group-bys positioned after visualizes and preserved that relative order when inserting Seer's new group-bys. This replacement unconditionally puts all group-bys first. So for columns where visualize leads (e.g. [count(message), service.name]), Seer will silently flip the order to [service.name, count(message)] every time it applies a result. Bug?
Was this meant to be checked? 🙂 |
There was a problem hiding this comment.
[Praise] I really appreciate you adding tests for this! (and, you know, extracting a shared testable helper in general)
I haven't reviewed these tests closely because I separately requested more refactoring. But I can if you want.
Extract reusable helpers (getLogsSeerLocationQuery, getLogsSeerAggregateFields) from the LogsTabSeerComboBox component so the Seer-to-URL wiring is testable and decoupled from the React component. Add support for Seer returning visualization payloads (chart type + y-axes) and propagate them into the aggregate field and sort query params. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When Seer returns a result that switches between samples and aggregate mode, stale sort and aggregate-field URL params from the previous mode persisted via the currentLocationQuery spread. Explicitly delete the opposite mode's keys so mode transitions produce a clean URL. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
isaacwang-sentry
force-pushed
the
codex/logs-ai-visualization-wiring
branch
from
9859122 to
b8b6a70
Compare
isaacwang-sentry
changed the base branch from
master
to
isaacwang/ref/deduplicate-seer-combobox-hooks
Extract shared Seer response mapping and Explore query shaping helpers so logs, spans, metrics, Discover, and Issues do not each carry their own copy of the same normalization logic. Co-Authored-By: Codex <noreply@openai.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 501b14c. Configure here.
Use nullable location updates when applying Seer responses so stale datetime query params are removed instead of serialized as empty values. Keep visualization chart type optional when Seer omits or returns an invalid value, preserving existing chart preferences instead of defaulting to line. Co-Authored-By: Codex <noreply@openai.com>
Make internal Seer query helpers private so knip does not flag them as unused exports. Add the required test Location key so the logs Seer combobox spec matches the current history Location type. Co-Authored-By: Codex <noreply@openai.com>
Clear logs pagination cursors whenever a Seer query is applied so stale sample or aggregate cursors cannot carry into the new result set. Preserve existing aggregate field ordering when inserting Seer group-bys and visualizations, including visualize-first columns. Co-Authored-By: Codex <noreply@openai.com>
3 participants
Summary
getLogsSeerLocationQueryandgetLogsSeerAggregateFieldsfrom theLogsTabSeerComboBoxcomponent into testable, reusable helpersTest plan
getLogsSeerLocationQuerycovering samples mode, aggregate mode, group-by ordering, and fallback when Seer omits visualizations