Skip to content
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

fix(logging): Add logging of change_dashboard_filter event for native dashboard filters #26333

Conversation

john-bodley
Copy link
Member

SUMMARY

For filter-box charts whenever the chart filters are changed we log an event per the LOG_ACTIONS_CHANGE_DASHBOARD_FILTER action—containing the filter-box chart ID and the associated filtered dataset columns—which is useful for performance logging.

The native dashboard filters log no such event and thus this PR simply logs the same action whenever anyone clicks the APPLY FILTERS button. I omitted both the chart ID and columns from the payload. The former is obsolete and the later is somewhat irrelevant as (unlike a filter-box chart) there are multiple datasets/columns impacted. Simply logging the event is suffice for tracking when the filters were updated resulting in a re-render of the dashboard.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Verified the action was logged to the logs table when the APPLY FILTERS button was clicked:

sqlite> SELECT json FROM logs WHERE json_extract(json, '$.event_name') = 'change_dashboard_filter' ORDER BY dttm;
{"source": "dashboard", "source_id": 2, "impression_id": "z8yL1F6XI", "version": "v2", "ts": 1703189280582, "event_name": "change_dashboard_filter", "event_type": "user", "event_id": "UXk2HJFqs", "visibility": "visible"}

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@john-bodley john-bodley force-pushed the john-bodley--fix-logging-change-dashboard-filter branch from ba0450a to fb367d4 Compare December 22, 2023 09:28
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (90a3d68) 69.18% compared to head (fb367d4) 69.18%.
Report is 6 commits behind head on master.

Files Patch % Lines
...board/components/nativeFilters/FilterBar/index.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #26333      +/-   ##
==========================================
- Coverage   69.18%   69.18%   -0.01%     
==========================================
  Files        1945     1945              
  Lines       75967    75969       +2     
  Branches     8467     8467              
==========================================
+ Hits        52556    52557       +1     
- Misses      21224    21225       +1     
  Partials     2187     2187              
Flag Coverage Δ
javascript 56.51% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -223,6 +225,7 @@ const FilterBar: React.FC<FiltersBarProps> = ({
}, [dashboardId, dataMaskAppliedText, history, updateKey, tabId]);

const handleApply = useCallback(() => {
dispatch(logEvent(LOG_ACTIONS_CHANGE_DASHBOARD_FILTER, {}));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe LOG_ACTIONS_CHANGE_DASHBOARD_FILTERS (plural)?

Copy link
Member Author

Choose a reason for hiding this comment

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

@michael-s-molina I though there was merit in keeping the action name the same for logging consistency.

Note that previously a single filter-box chart could actually have multiple filters so the singular term was also likely misleading then as well.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Left a non-blocking suggestion.

@john-bodley john-bodley merged commit 5f5a656 into apache:master Dec 26, 2023
30 checks passed
@john-bodley john-bodley deleted the john-bodley--fix-logging-change-dashboard-filter branch December 26, 2023 19:48
@michael-s-molina michael-s-molina added v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch labels Dec 27, 2023
michael-s-molina pushed a commit that referenced this pull request Dec 27, 2023
michael-s-molina pushed a commit that referenced this pull request Dec 27, 2023
josedev-union pushed a commit to Ortege-xyz/studio that referenced this pull request Jan 22, 2024
@mistercrunch mistercrunch added 🍒 3.0.3 🍒 3.0.4 🍒 3.1.0 🍒 3.1.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Mar 8, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch v3.1 Label added by the release manager to track PRs to be included in the 3.1 branch 🍒 3.0.3 🍒 3.0.4 🍒 3.1.0 🍒 3.1.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants