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

[SIP-64] Migrate filter_box to Dashboard Native Filter Component #14383

Closed
10 of 11 tasks
graceguo-supercat opened this issue Apr 28, 2021 · 12 comments
Closed
10 of 11 tasks
Assignees
Labels
dashboard:native-filters Related to the native filters of the Dashboard sip Superset Improvement Proposal

Comments

@graceguo-supercat
Copy link

graceguo-supercat commented Apr 28, 2021

[SIP] Proposal for migrating filter_box viz to dashboard native filter component

Motivation

This is the last step to complete the dashboard filter components. Please see discussion about dashboard filter components.

With the dashboard native filter development become stable, Superset should migrate from filter_box to dashboard native filter components, and prepare to expire filter_box charts. As one of the key component for Superset dashboard in the last 4+ years, and given the big varieties of use cases for different users, this migration will need combination of preview, automatic scripts and manual adjustment, and DB migration.

Note

  • The goal of this migration plan is to migrate filter_box used in dashboard to native_filter_configuration dashboard metadata, and saved into dashboard.
  • This migration plan do not change the dashboard layout. After dashboard starts to use native filter only, dashboard owner could remove filter_box from their dashboard manually, and reorganize dashboard layout.

Proposed Change

Filter_box migration flow(3)

Precondition

Superset is actively used by at least hundreds of users in many companies as their data analytics tool. In order to make sure that majorities of Superset users' work are not lost, damaged by this filter migration, I feel it is very important to make sure we complete the following Precondition check:

Transition Mode

Once we resolved the feature parity and performance downgrade issue, it is safe to start migrate.

I group users into 2 type and migration solution is a little different:

airbnb user

Not enabled native filter feature flag at the beginning of migration. These users do not have native_filter_configuration in the dashboard metadata. I propose this migration process:

  1. When Dashboard owner open their dashboard, we offer a function in the front-end (JS only), which read config from each filter_box and automatically convert them into the data compatible with native_filter_configuration. Dashboard will seamlessly display filter components from converted filter config. At the same time, dashboard will disable filter_box chart and dashboardFilter related front-end logic to make sure didn't make unnecessary requests.

  2. Now Dashboard owner should see how filter components work without much manual work. They should check if filter component generate correct filter value, if filter has right scope, etc,. This is to verify the logic of front-end migration valid and works well.

  3. If Dashboard owner find some issues for the converted version, or any UI/UX feedback, concerns, they can report issue. Dashboard will keep intact no change.

  4. If Dashboard owner do not want to verify right now, I also offer a snooze mode, which allow user set a period of time and Dashboard will not be converted when user open it. After the snooze time, dashboard will be automatically converted again.

  5. If Dashboard owner like the converted dashboard, they can choose SAVE dashboard before they leave. After that this dashboard will have good native_filter_configuration in dashboard metadata. And when owner opens dashboard next time, we don't need to run front-end migration logic.

Note: There is a tricky part: when Dashboard owner is reviewing JS-converted filters, he/she probably can not save other changes without save native_filter_configuration. We should show clear confirm message to let owners know what content get saved.

  1. Dashboard viewers (do not have can_edit permission for the dashboard), will not see JS-converted dashboard during transition mode. After dashboard owners saved the change, viewers can see and use filter components.
advanced users

Already enabled native filter feature flag at the beginning of migration. These users already manually created native filter in their dashboard so that they had native_filter_configuration metadata.

  1. For these users, they already had filter components in the dashboard, but may also have some old filter_box. So they already knew how filter component work, and there is no data or functionality lost concerns. These users are like above user after Step 5.

  2. Given they already had created valuable native_filter_configuration, we should not add automatic converted content into this metatdata.

  3. Dashboard owners have to manually create filter components to replace their filter_box. In Transition mode, all filter_box will stop working in the dashboard (as long as dashboard has native_filter_configuration metadata).

DB Migration

After large number of dashboard filter_box are safely migrated to native filter components, we can move to DB Migration step.

During DB migration, there will be a python function that walk through all dashboards and change its metadata. It will do complete following jobs:

  • Find dashboard without native_filter_configuration: find its filter_box config and convert them into native_filter_configuration, and saved into dashboard metadata.
  • From dashboard_slices table, remove the all relationship entries for filter_box and dashboard.

Then we can remove all filter_box viz from slices table, remove front-end JS convert logic, and etc.

New or Changed Public Interfaces

  • Convert filter_box config properties into dashboard native filter metadata.
  • Dashboard will only offer interactive filtering functionalities by filter components.
  • Remove filter_box charts from all dashboard.
  • Expire filter_box viz_type from Superset system.

New dependencies

See Precondition section above for more details.

Migration Plan and Compatibility

Based on your dashboard complexity and filter component usage, corporation/company can optionally kickoff migration mode after this PR merged.

Migration mode is optional. In the end all filter_box used in the dashboard, will be converted into filter component by DB migration script (this will be created later by separated PR). And from then on Superset will not expire filter_box viz type in our codebase.

Rejected Alternatives

N/A

@graceguo-supercat graceguo-supercat added the sip Superset Improvement Proposal label Apr 28, 2021
@junlincc junlincc added the dashboard:native-filters Related to the native filters of the Dashboard label Apr 28, 2021
@amitmiran137
Copy link
Member

I think that we should have it as an optional migration tool to run by demand, like a CLI command or a rest endpoint.
the reason for that is that dashboard owners need to decide on how to design their dashboard after the removal of a filter box.
the location of the filter box on the dashboard matters for them, so moving away from filter box changes how they design the look and feel of the dashboard

I can make sure that everyone migrates to the native-filters manually bc we don't have tons of dashboards yet!

@graceguo-supercat graceguo-supercat changed the title [SIP-64] Migrate Filter_box to Dashboard Native Filter Component [WIP][SIP-64] Migrate Filter_box to Dashboard Native Filter Component Apr 28, 2021
@graceguo-supercat graceguo-supercat changed the title [WIP][SIP-64] Migrate Filter_box to Dashboard Native Filter Component [SIP-64] Migrate Filter_box to Dashboard Native Filter Component Apr 30, 2021
@graceguo-supercat
Copy link
Author

graceguo-supercat commented Apr 30, 2021

I think that we should have it as an optional migration tool to run by demand, like a CLI command or a rest endpoint.

@amitmiran137 I assume your request is to let each dashboard owner run the CLI command? What state is the dashboard currently in, and what state they want to achieve?
If they already had good native filter configuration, they can re-arrange dashboard layout any time. My proposal does not cover re-arrange dashboard layout solution, and i assume it must be done manually.

@john-bodley john-bodley changed the title [SIP-64] Migrate Filter_box to Dashboard Native Filter Component [SIP-64] Migrate filter_box to Dashboard Native Filter Component Jul 19, 2021
@michael-s-molina
Copy link
Member

@graceguo-supercat If in the end, we’ll have a back-end migration script that converts a filter box configuration into a native filter configuration, why don’t we skip the part of creating the same logic on the front-end and just do it on the back-end? We can add to each automatically converted filter the status of approved or rejected (which will hide the filter) and offer an option to the owner to set this status on the interface. Something like this:

Screen Shot 2021-09-21 at 4 45 01 PM

This way we can monitor rejected filters, migration completion, and the owner can save his dashboard without worrying about side effects. Only accepted filters will appear to non-owner users. This workflow also works for users that already have native filters created.

The owner can have both filter boxes and native filters at the same time for validation, and when it is validated, he can remove the filter box from the dashboard or we can do it on a pre-defined date.

@graceguo-supercat
Copy link
Author

graceguo-supercat commented Sep 22, 2021

If in the end, we’ll have a back-end migration script that converts a filter box configuration into a native filter configuration, why don’t we skip the part of creating the same logic on the front-end and just do it on the back-end?

In airbnb our DS use Superset do critical jobs. In case any automatically convert didn't work as expected, or after convert to filter components, dashboard is too slow to use etc, we need to unblock our users ASAP so that at least, they can still use old filter_box to complete their work.

@stale
Copy link

stale bot commented Apr 17, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Apr 17, 2022
@jhult
Copy link
Contributor

jhult commented Apr 20, 2022

Should this stay open?

@stale stale bot removed the inactive Inactive for >= 30 days label Apr 20, 2022
@regisb
Copy link
Contributor

regisb commented Apr 20, 2022

Should this stay open?

I... think so?

@rusackas
Copy link
Member

Technically the vote for the SIP is still open, which is why the thread is too. Hopefully we can resolve this soon. It's peculiar since there are more than enough votes to close the SIP, which @graceguo-supercat might want to do, but there's also some talk about the next phase(s) of dashboard filters and how it may further impact this migration plan, so I could also see leaving it open until things are final final.

@abhiroop93
Copy link

Hey,
I am also facing this issue while upgrading from 1.0.0 to 1.4.2.
My older dashboards used filter boxes and upon upgrading, the dashboards don't seem to work anymore because of the new native filters. Is there a way to get this working? Any way to migrate older filter boxes? @rusackas @graceguo-supercat @michael-s-molina

@john-bodley
Copy link
Member

john-bodley commented Oct 28, 2022

Primer

Per #14383 (comment) although the work has been partially completed (per #16992), this SIP has not been voted on and thus I was hoping to share some recent observations combined with a proposed change as to how the migration logic could work, which will then serve as the seed for a formal discussion and potential vote.

Observations

The dashboard native filters and frontend filter-box migration features are controlled via the DASHBOARD_NATIVE_FILTERS (defaults to true) and ENABLE_FILTER_BOX_MIGRATION (defaults to false) feature flags respectively. The following observations relate to having the respective feature flag enabled.

DASHBOARD_NATIVE_FILTERS
  1. The native filters don't always show up by default. This is most likely related to (2).
  2. Simply viewing a dashboard without the feature enabled saves the dashboard which is undesirable and has the potential of polluting Superset's metadata. This is likely due to the addition of the show_native_filters JSON parameter. This key is likely superfluous—I hypothesize it’s leveraged by the migration workflow—and likely can be removed.
  3. Though not ideal from a UX perspective, legacy filter scopes and coexist with the native filter scopes. Note if the dashboard does not have any filter-box charts then —rightfully so—the “Set filter mapping” option does not exist.
  4. One can still add a legacy filter-box chart to the dashboard. The only way to (partially) prevent this is to enable the ENABLE_FILTER_BOX_MIGRATION feature.
  5. The GET /api/v1/dataset/{pk} RESTful API endpoint is sub-performant for very large datasets making the feature somewhat unusable. See this Slack thread in the #project-native-filters channel.
ENABLE_FILTER_BOX_MIGRATION
  1. It’s not apparent what the review process is, i.e., how to accept or revert the change. Note I believe that the intended flow is that these native filters are automatically persisted without any course to revert—refer to (2) as to why this isn’t the case—which makes the term “review process” somewhat of a misnomer.
  2. The feature seems to be broken in master as one is unable to save. Per the video here there’s no obvious way of how to persist the migration, i.e., there’s no save button or way to exit the workflow, though it’s worth adding that a pencil icon exists in the filter bar which is no longer present.
  3. Post migration though the filter-box charts are grayed out they still remain functional (albeit being a duplicate). Furthermore the filter pill etc. is enabled and potentially misleading as they’re now deemed as a chart which is governed by a native filter i.e., the UX isn’t ideal given the confusion.
  4. The feature is not sufficient in the sense that only the dashboards where the owners completed the review are updated. A one off CLI migration (which doesn’t currently exist) still needs to be added.
  5. Even though you can’t add a filter-box chart to the dashboard via the dashboard edit flow you still can—via explore— either i) create a filter-box chart, or ii) add a filter-box chart to a dashboard on save.

Recommendations

  1. Optimize the /api/v1/dataset/{pk} RESTful API endpoint and add selective column filtering to improve performance.
  2. Remove the ENABLE_FILTER_BOX_MIGRATION feature. Though a frontend migration workflow has merit there's no specific call to action and/or mechanism to revert the migration. Furthermore the rubric outlined in apache/superset#16992 as part of the transition mode with the various scenarios is overly complex and can be greatly simplified. The TL;DR is one simply enables native filters—which enables the coexistence of both native and legacy filters—then some time later a migration is performed which migrates any remaining legacy filters to their native form.
  3. Remove the superfluous show_native_filters JSON parameter given that the DASHBOARD_NATIVE_FILTERS feature flag should be suffice. This will prevent dashboards from being saved on view if a filter-box chart is present.
  4. Disable creating or adding filter-box charts to a dashboard (via the various flows: from within a dashboard, creating a new filter-box chart, saving a filter-box chart to a dashboard, or possibly importing a previously exported dashboard) when the DASHBOARD_NATIVE_FILTERS feature is enabled.
  5. Disable editing the filter mapping if the DASHBOARD_NATIVE_FILTERS feature is enabled and no dashboard level filter scopes are defined—if undefined filters are applicable to all charts, i.e., there's no immunity. This will prevent users from defining both legacy and native filters.
  6. Perform a two-phase migration (to help ensure a smoother transition as well as providing a method of recourse):
    • Author a CLI script to migrate dashboards. Assuming that the DASHBOARD_NATIVE_FILTERS feature is enabled, the script will:
      • Migrate all embedded filter-box charts to native filters irrespective of whether there already exists native filters on a dashboard. One would preferably run the script soon after the feature is enabled to reduce the coexistence of both native and legacy filters which have their own scoping mechanism. Note per (2) only native filters can be added to a new or existing dashboard which means once migrated a dashboard cannot regress to having both native and legacy filters.
      • Uniquify the filter names (which is currently not the case) leveraging the chart title if the filterable element exists within multiple filter-box charts within the dashboards.
      • Keep the legacy filter scopes in the dashboard metadata which will allow for the migration to be reverted.
      • Use a markdown placeholder where the previous filter-box chart resided. This ensures that i) the layout of the dashboard is preserved, and ii) all filter-box charts are actually removed (as opposed to disabled) from the dashboard. The markdown will include the name and link to the deprecated filter-box chart for reference. Note these markdown components can only ever be removed by the dashboard owner(s).
    • Author a CLI script to delete (clean up) all filter-box charts and legacy filter scopes—which by definition of (4) and (5) are not associated with any dashboards. This should be run by an admin after sufficient time has elapsed to revert/address any potential regressions.
  7. Once the filter-box charts are removed an Alembic migration will need to be added which invokes the CLI scripts.

@john-bodley
Copy link
Member

Per the email I sent on 23-11-2022 titled "[RESULT][VOTE][SIP-64] Migrate filter_box to Dashboard Native Filter Component", The vote has passed with seven +1 binding votes, zero +0 votes, and zero -1 votes.

@rusackas
Copy link
Member

rusackas commented Feb 7, 2024

@michael-s-molina is the actual work here complete so we can move it to the DONE column? Seems like it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dashboard:native-filters Related to the native filters of the Dashboard sip Superset Improvement Proposal
Projects
Status: Implemented / Done
Development

No branches or pull requests

9 participants