Skip to content

feat: disable db and coll stats via preferences COMPASS-5387 #6852

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

Merged
merged 21 commits into from
Apr 30, 2025

Conversation

paula-stacho
Copy link
Contributor

@paula-stacho paula-stacho commented Apr 9, 2025

https://jira.mongodb.org/browse/COMPASS-5387

Description

The stats are enabled by default and can be turned off in the configuration.

This affects views:

Menu

Screenshot 2025-04-14 at 14 08 21

Databases list & grid

Screenshot 2025-04-14 at 14 07 27

Collections list & grid

Screenshot 2025-04-14 at 14 13 29

Collection -> Documents tab

Screenshot 2025-04-14 at 14 16 06

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added
  • If this change updates the UI, screenshots/videos are added and a design review is requested
  • I have signed the MongoDB Contributor License Agreement (https://www.mongodb.com/legal/contributor-agreement)

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@github-actions github-actions bot added the feat label Apr 14, 2025
@paula-stacho paula-stacho marked this pull request as ready for review April 14, 2025 12:16
@paula-stacho paula-stacho changed the title feat: disable db and coll stats via preferences feat: disable db and coll stats via preferences COMPASS-5387 Apr 14, 2025
* @returns
*/
async fetch({ dataService, fetchInfo = true, force = false }) {
if (!shouldFetch(this.status, force)) {
return;
}

const instanceModel = getParentByType(this, 'Instance');
const { enableDbAndCollStats } = instanceModel.preferences.getPreferences();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of exposing all the preferences directly from the instance model, can we limit the exposed interface to only what we need here? Something like:

Suggested change
const { enableDbAndCollStats } = instanceModel.preferences.getPreferences();
const enableDbAndCollStats = instanceModel.shouldAllowStatsFetching();

As a rule of thumb I'd suggest to try to stay minimal when introducing these sort of cross boudary interfaces so that the intent of the exposed interface is clearer from just the name and the actual implementation can be changed easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good idea, I'll do that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I ended up using a property instead of a function, as I wanted to make sure we listen to updates on the preferences.

@paula-stacho paula-stacho force-pushed the COMPASS-5387 branch 2 times, most recently from 9133126 to cf4086f Compare April 24, 2025 15:31
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

Changes look good, one question on a possible improvement

Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

lgtm! Nice work on this, big preference add.

@paula-stacho paula-stacho merged commit f1a3722 into main Apr 30, 2025
77 of 79 checks passed
@paula-stacho paula-stacho deleted the COMPASS-5387 branch April 30, 2025 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants