-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
rabbit_feature_flags: Rework the management UI page #12643
Conversation
As of this writing, the branch contains a commit that adds three feature flags just to help test the branch. They will be removed before the merge. These feature flags are:
They all use an CI will fail until they are removed because it will fail to enable |
34e7b18
to
22835c7
Compare
This looks great. |
ad6dbd4
to
82cb218
Compare
There are two issues related to auto-refresh:
Both issues are relatively minor and the overall experience is already better. I don't insist we have to fix these. |
…orted or not [Why] Durint the development of Khepri, it was difficult to communicate that it was unsupported in RabbitMQ 3.13.x but was then supported in 4.0.x even though it was still experimental. [How] The feature flag definition now exposes that support level in a now attribute called `experiment_level`. It can be `unsupported` or `supported`. We can use this now attribute in the CLI or the web UI to convey the level of support to the end user. In the future, we could imagine that an experimental feature flag becomes abandoned, where upgraded from a node that has it enabled to a version that marks the feature flag as abandoned is not possible.
…gement API [Why] It allows to better communicate each feature flag specificities and make a better more user-friendly management UI.
[Why] The previous implementation was using the blocking `is_enabled/1` API. This meant that if a feature flag was being enabled and the enable callback took time, the CLI's `list_feature_flag` command or any use of the management UI would block until the feature flag was enabled. [How] `get_state/1` now uses the non-blocking API. However it returns a now possible value: `state_changing`.
[Why] The "Feature flags" admin section had several issues: * It was not designed for experimental feature flags. What was done for RabbitMQ 4.0.0 was still unclear as to what a user should expect for experimental feature flags. * The UI uses synchronous requests from the browser main thread. It means that for a feature flag that has a long running migration callback, the browser tab could freeze for a very long time. [How] The feature flags table is reworked and now displays: * a series of icons to highlight the following: * a feature flag that has a migration function and thus that can take time to be enabled * a feature flag that is experimental * whether this experimental feature flag is supported or not * a toggle to quickly show if a feature flag is enabled or not and let the user enable it at the same time. For stable feature flags, when a user click on the toggle, the toggle goes into an intermediate state while waiting for the response from the broker. If the response is successful, the toggle is green. Otherwise it goes back to red and the error is displayed in a popup as before. For experimental feature flags, when a user click on the toggle, a popup is displayed to let the user know of the possible constraints and consequences, with one or two required checkboxes to tick so the user confirms they understand the message. The feature flag is enabled only after the user validates the popup. The displayed message and the checkboxes depend on if the experimental feature flag is supported or not (it is a new attribute of experimental feature flags). The request to enable feature flags now uses the modern `fetch()` API. Therefore it uses Javascript promises and does not block the main thread: the UI remains responsive while a migration callback runs. Finally, an "Enable all stable feature flags" button has been added to the warning that tells the user some stable feature flags are still disabled. V2: Pause auto-refresh while a feature flag is being handled. This fixes some display inconsistencies.
82cb218
to
f7a740c
Compare
@mkuratczyk: I finally found and implemented a fix for the issues you list where the auto-refresh is involved: I pause the auto-refresh timer and resume it at the end of handling feature flags. I think there are still issues in this area. For example, if the user switches to another page and comes back. Or if they play with the auto-refresh dropdown menu at the top right. But I feel fixing this becomes out of scope for this patch. We can wait that a user reports them :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great now
Why
The "Feature flags" admin section had several issues:
How
The feature flags table is reworked and now displays:
Here is how the new table looks like:
For stable feature flags, when a user click on the toggle, the toggle goes into an intermediate state while waiting for the response from the broker. If the response is successful, the toggle is green. Otherwise it goes back to red and the error is displayed in a popup as before.
For experimental feature flags, when a user click on the toggle, a popup is displayed to let the user know of the possible constraints and consequences, with one or two required checkboxes to tick so the user confirms they understand the message. The feature flag is enabled only after the user validates the popup. The displayed message and the checkboxes depend on if the experimental feature flag is supported or not (it is a new attribute of experimental feature flags).
Here is a screenshot of the confirmation popup for unsupported experimental feature flags:
The request to enable feature flags now uses the modern
fetch()
API. Therefore it uses Javascript promises and does not block the main thread: the UI remains responsive while a migration callback runs.Finally, an "Enable all stable feature flags" button has been added to the warning that tells the user some stable feature flags are still disabled.
Here is a screenshot of the warning with the added button: