chore: add logging event for drill to detail - #42563
Conversation
Drill By is instrumented with four log events (drill_by_modal_opened,
further_drill_by, drill_by_edit_chart, drill_by_breadcrumb_clicked), but its
sibling feature Drill to Detail has none, so there is no way to tell that a user
opened Drill to Detail, or on which chart.
It cannot be reconstructed server-side either. Opening the modal issues a single
request, POST /datasource/samples, whose body is built by getDrillPayload() as
{granularity, time_range, filters, extras} with query args
datasource_type/datasource_id/dashboard_id/force/page/per_page — none of which
reference a chart. Since one dataset commonly backs many charts on the same
dashboard, datasource_id + dashboard_id cannot identify the chart.
DatasetRestApi.get_drill_info does log, but it is not a drill signal:
useDatasetDrillInfo prefetches it when the chart renders, so it emits a row per
dataset per dashboard load even when the user never drills, and carries only pk
(the dataset id) and rison.dashboard_id.
Adds LOG_ACTIONS_DRILL_TO_DETAIL_MODAL_OPENED, dispatched from DrillDetailModal
with slice_id, mirroring DrillByModal. This covers both entry points — the chart
header menu and the right-click context menu — since both render this modal.
Unlike DrillByModal, which mounts when opened, DrillDetailModal stays mounted
and is toggled via the showModal prop, so the effect is gated on showModal to
avoid firing on mount while the modal is closed.
dosubot
Bot
added
dashboard:drill-to-detail
logging
Code Review Agent Run #32bb1eActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| useEffect(() => { | ||
| if (showModal) { | ||
| dispatch( | ||
| logEvent(LOG_ACTIONS_DRILL_TO_DETAIL_MODAL_OPENED, { | ||
| slice_id: chartId, | ||
| }), | ||
| ); | ||
| } | ||
| }, [dispatch, showModal, chartId]); |
There was a problem hiding this comment.
Suggestion: Because chartId is an effect dependency, changing the chart identity while the persistent modal remains open reruns this effect and records another “opened” event without a new open transition. Track the previous showModal value and log only when it changes from false to true, while using the current chart ID for that transition. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Duplicate Drill to Detail analytics rows can be recorded.
- ⚠️ Chart-level modal-open metrics may overcount user openings.
- ⚠️ Dashboard logging becomes inaccurate when chart identity changes in-place.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx
**Line:** 117:125
**Comment:**
*Stale Reference: Because `chartId` is an effect dependency, changing the chart identity while the persistent modal remains open reruns this effect and records another “opened” event without a new open transition. Track the previous `showModal` value and log only when it changes from false to true, while using the current chart ID for that transition.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Thanks for the review. I looked into this and I don't believe it's reachable with the current usage, so I'd rather not add the extra state.
DrillDetailModal is rendered from two places, and in both the chartId is fixed for the lifetime of the mounted instance:
SliceHeaderControlsrenders it withchartId={slice.slice_id}, and that component is rendered per slice.ChartContextMenurenders it withchartId={id}, and that component is rendered per chart insideChartRenderer.
So there is no path where a mounted instance's chartId changes while showModal stays true. On top of that the modal is modal — it blocks interaction with the dashboard underneath while open — so a chart swap under an open modal isn't reachable either.
Given that, a useRef transition guard would be extra state to keep in sync with no behavioural difference today. I'd prefer to keep the effect as it is and revisit if a future caller ever shares one modal instance across charts.
Worth noting the suggested snippet also initialises the ref with useRef(showModal), which would swallow the event for a component that mounts with the modal already open. If we do add a guard later, it should start from false.
The existing tests cover the behaviour: one asserts the event fires with the correct slice_id when the modal is opened, and another asserts nothing is logged while it is closed.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag duplicate modal-open analytics caused by chart identity changes when each modal instance has a chart ID fixed for its lifetime; only flag this when a modal instance can be reused across charts while remaining open.
Applied to:
superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx
💡 To manage or update this instruction, visit: CodeAnt AI Settings
|
The flagged issue is correct. Because Here is the corrected implementation for const prevShowModal = useRef(showModal);
useEffect(() => {
if (showModal && !prevShowModal.current) {
dispatch(
logEvent(LOG_ACTIONS_DRILL_TO_DETAIL_MODAL_OPENED, {
slice_id: chartId,
}),
);
}
prevShowModal.current = showModal;
}, [dispatch, showModal, chartId]);Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42563 +/- ##
==========================================
- Coverage 65.33% 65.33% -0.01%
==========================================
Files 2803 2803
Lines 158490 158495 +5
Branches 36178 36179 +1
==========================================
+ Hits 103557 103559 +2
- Misses 52922 52924 +2
- Partials 2011 2012 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds frontend instrumentation for the Drill to Detail feature by emitting a dedicated log event when the modal is opened, enabling analytics to identify when a drill occurs and which chart (slice) initiated it.
Changes:
- Added a new log action constant:
drill_to_detail_modal_opened. - Dispatched the new
logEventfromDrillDetailModalgated byshowModal(so it only fires on open, not on mount). - Added unit tests verifying the event fires when opened and does not fire while closed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| superset-frontend/src/logger/LogUtils.ts | Adds the new LOG_ACTIONS_DRILL_TO_DETAIL_MODAL_OPENED constant. |
| superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.tsx | Emits drill_to_detail_modal_opened with { slice_id: chartId } when showModal becomes true. |
| superset-frontend/src/components/Chart/DrillDetail/DrillDetailModal.test.tsx | Adds tests to assert the event is logged on open and not logged while closed. |
SUMMARY
Drill By is instrumented with four log events (added in #23854):
drill_by_modal_opened,further_drill_by,drill_by_edit_chartanddrill_by_breadcrumb_clicked. Its sibling feature Drill to Detail has none, so there is no way to tell from thelogstable that a user opened Drill to Detail, or on which chart.It cannot be reconstructed server-side either. Opening the modal issues exactly one request,
POST /datasource/samples, whose body is built bygetDrillPayload()as{granularity, time_range, filters, extras}and whose query args aredatasource_type,datasource_id,dashboard_id,force,page,per_page— none of which reference a chart. Because one dataset commonly backs many charts on the same dashboard,datasource_id + dashboard_idis not enough to identify the chart.DatasetRestApi.get_drill_infodoes write a log row, but it is not a drill signal:useDatasetDrillInfoprefetches it when the chart renders (gated only oncanDrillToDetail), so it emits one row per dataset per dashboard load even when the user never drills, and its payload carries onlypk(the dataset id) andrison.dashboard_id.This PR adds
LOG_ACTIONS_DRILL_TO_DETAIL_MODAL_OPENED(drill_to_detail_modal_opened), dispatched fromDrillDetailModalwithslice_id, mirroringDrillByModal. Because both entry points — the chart header ⋮ menu and the right-click context menu — render this same modal, one dispatch covers both.Implementation note. Unlike
DrillByModal, which mounts when it is opened,DrillDetailModalstays mounted for the lifetime of the chart and is toggled via itsshowModalprop. The effect is therefore gated onshowModalrather than firing on mount, so no event is emitted while the modal is closed. Reopening the modal emits a new event, which is the intended behaviour.DrillDetailModalalready receiveschartIdas a prop and already resolves the chart name fromsliceEntitiesto render its own title, so no new data plumbing was needed.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable — no UI change.
TESTING INSTRUCTIONS
Two unit tests are added to
DrillDetailModal.test.tsx:should log an event when the modal is opened— assertslogEventis called withdrill_to_detail_modal_openedand{ slice_id }.should not log an event while the modal is closed— renders withshowModal={false}and asserts nothing is logged.Manually:
drill_to_detail_modal_openedevent is recorded with the correctslice_id— visible in the/superset/log/request payload, or in thelogstable withEVENT_LOGGERconfigured.ADDITIONAL INFORMATION