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

Highlight values that are different from the default in cluster settings #76626

Closed
lidorcarmel opened this issue Feb 15, 2022 · 13 comments · Fixed by #78034
Closed

Highlight values that are different from the default in cluster settings #76626

lidorcarmel opened this issue Feb 15, 2022 · 13 comments · Fixed by #78034
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@lidorcarmel
Copy link
Contributor

lidorcarmel commented Feb 15, 2022

Is your feature request related to a problem? Please describe.
It's hard to see which cluster settings currently have a value which is different from the default when using the UI.

Describe the solution you'd like
When navigating to the cluster settings page (under /#/reports/settings), have a view that only shows the cluster settings that have a value different from the default, and another view for all settings as we have today. Probably the default view should be the new one.

Describe alternatives you've considered
I guess highlighting the values that are different from the default is also an option, but it's along scroll so it's easy to miss. In practice I think the main reason to look at settings is to get those values. The defaults are rarely interesting IMO.

Additional context
This is how it looks now:
Screen Shot 2022-02-15 at 11 39 53 AM

Jira issue: CRDB-13198

Epic CRDB-13054

@lidorcarmel lidorcarmel added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Feb 15, 2022
@Santamaura
Copy link
Contributor

Hi @lidorcarmel I have started looking into this improvement and in order to move forward we need to update the endpoint with the overridden status of the settings. I'm not too familiar with settings but do you see a way to add the statuses from here:

Public: v.Visibility() == settings.Public,
?

@Santamaura
Copy link
Contributor

Would using

OverridesInformer OverridesInformer
work?

@tbg
Copy link
Member

tbg commented Mar 16, 2022

internal slack thread

@Santamaura
Copy link
Contributor

Santamaura commented Mar 16, 2022

@thtruo @Annebirzin For the FE design side of things Lidor mentioned having a separate view so would it make sense in this case to have like a same-style table above the current one which just lists the altered settings? I'm open to thoughts/suggestions

@lidorcarmel
Copy link
Contributor Author

another option can be to have 2 separate tabs maybe? one for the settings that have a value different from the default and one tab for everything.

@lidorcarmel lidorcarmel changed the title Highlight overridden cluster settings Highlight values that are different from the default in cluster settings Mar 16, 2022
@lidorcarmel
Copy link
Contributor Author

changed the language of this issue to not use "overridden", it's a bit awkward but hopefully less confusing.
and thanks Alex for looking into this!!

@Annebirzin
Copy link

Annebirzin commented Mar 16, 2022

@Santamaura @lidorcarmel I was just looking into designs for this issue. I'm thinking maybe we could keep everything in one table, but update the table to add a column for 'Last override date'.

The default sort would be by override date (rather than alphabetical as it is today), so all settings with overrides would appear at the top (most recent override first). The 'Setting' column could also include an alphabetical sort so users can still see settings alphabetically.

These designs would also include an update to our current table styles (including adding the column sort option).

thoughts?

Screen Shot 2022-03-16 at 4 10 36 PM

cc @thtruo

@thtruo
Copy link
Contributor

thtruo commented Mar 16, 2022

I had a conversation with TSEs just now - they reacted positively to the proposed designs. They're okay with having the "last override date" and sorting the view to see all non-defaults at the top.

@ajwerner
Copy link
Contributor

ajwerner commented Mar 16, 2022

Designs which require adding new state to the database (like when something was set) are great, but are also take much longer as we'll need to wait a major version release and even then we won't have this data moving backwards.

For something we can do quickly and use quickly, we have a boolean and just a boolean.

update: turns out we already track it

@Santamaura
Copy link
Contributor

For something we can do quickly and use quickly, we have a boolean and just a boolean.

If we instead do SELECT * FROM system.settings we could grab the 3rd column (which is the last updated TS ) and return that couldn't we?

@ajwerner
Copy link
Contributor

😓 I'm sorry, I didn't realize we had that. I appologize.

@Santamaura
Copy link
Contributor

No problem 😄

@lidorcarmel
Copy link
Contributor Author

having the last override date sounds super useful, to see if the change time correlates with the issue we're debugging. and of course one table is better than 2.. thank you :)

Santamaura added a commit to Santamaura/cockroach that referenced this issue Mar 21, 2022
Resolves cockroachdb#76626

Previously it was difficult to determine from the cluster
settings page which settings have been altered.
This patch updates the cluster settings endpoint to include a
last updated field, indicating when the setting was last altered,
which allows the settings page to include that column in the
table and is pre-sorted by this value.

Release note: None

Release justification:
Release note (<category, see below>): <what> <show> <why>
Santamaura added a commit to Santamaura/cockroach that referenced this issue Mar 21, 2022
Resolves cockroachdb#76626

Previously it was difficult to determine from the cluster
settings page which settings have been altered.
This patch updates the cluster settings endpoint to include a
last updated field, indicating when the setting was last altered,
which allows the settings page to include that column in the
table and is pre-sorted by this value.

Release note: None

Release justification:
Release note (<category, see below>): <what> <show> <why>
craig bot pushed a commit that referenced this issue Mar 28, 2022
77642: sql: backend changes to surface unused index recommendations r=THardy98 a=THardy98

This change introduces backend changes to surface drop index
recommendations for unused indices. As a first iteration, this change
implements a naive approach where we compute index recommendations
ad-hoc on request (for the database details, table details, and index
usage stats requests), and offers no persistence.

Release note (api change): Added logic to support dropping unused index
recommendations.

78034: [CRDB-13198] ui, server: update cluster settings to include last update time r=Santamaura a=Santamaura

Resolves #76626

Previously it was difficult to determine from the cluster
settings page which settings have been altered.
This patch updates the cluster settings endpoint to include a
last updated field, indicating when the setting was last altered,
which allows the settings page to include that column in the
table and is pre-sorted by this value.

Release note: None

Release justification: Fairly minor changes for QoL upgrade

![Screen Shot 2022-03-21 at 10 42 27 AM](https://user-images.githubusercontent.com/17861665/159289222-28970bbe-cac0-4003-853b-35e619908f0f.png)

78532: clusterversion: introduce version key for `crdb_internal.cluster_locks` r=AlexTalks a=AlexTalks

This change adds a new cluster version to enable usage of the virtual
table `crdb_internal.cluster_locks`, so that we can disable usage of new
KV `QueryLocksRequest` API on cluster versions prior to 22.1.

Release note: None

Release justification: introduces a new cluster version without any
logic changes

78631: lease: add a defensive check for nil-ness r=Xiang-Gu a=Xiang-Gu

Previously, return from mananger.findDescriptorState is used directly
without checking nil-ness. It seems the original author assumed the
that return will never be nil. It will be safer to check nil-ness.

Release note: None

Co-authored-by: Thomas Hardy <[email protected]>
Co-authored-by: Santamaura <[email protected]>
Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Xiang Gu <[email protected]>
@craig craig bot closed this as completed in 2ded655 Mar 29, 2022
Santamaura added a commit to Santamaura/cockroach that referenced this issue May 26, 2022
Resolves cockroachdb#76626

Previously it was difficult to determine from the cluster
settings page which settings have been altered.
This patch updates the cluster settings endpoint to include a
last updated field, indicating when the setting was last altered,
which allows the settings page to include that column in the
table and is pre-sorted by this value.

Release justification: Fairly minor changes for QoL upgrade
Release note: None
Santamaura added a commit to Santamaura/cockroach that referenced this issue Jun 1, 2022
Resolves cockroachdb#76626

Previously it was difficult to determine from the cluster
settings page which settings have been altered.
This patch updates the cluster settings endpoint to include a
last updated field, indicating when the setting was last altered,
which allows the settings page to include that column in the
table and is pre-sorted by this value.

Release justification: Fairly minor changes for QoL upgrade
Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants