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

Add options for History and Wanted/Missing queries skip, remove sort on Wanted/Missing #299

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

codefaux
Copy link

@codefaux codefaux commented May 9, 2024

Description of the change

  • Add options for History and Wanted/Missing queries skip, remove sort on Wanted/Missing

Benefits

  • Faster metrics collection for users not desiring History/Wanted/Missing
  • Stall mitigation re: users with large collections or long-lived instances

Possible drawbacks

  • I don't know Golang or koanf and did not look up reference for either, but I believe I have treated both appropriately.

Applicable issues

Additional information

  • Thanks for your work.

@codefaux codefaux requested a review from onedr0p as a code owner May 9, 2024 00:52
Comment on lines 26 to 27
flags.Bool("skip-history", false, "Skip History metrics query")
flags.Bool("skip-missing", false, "Skip Wanted/Missing metrics query")
Copy link
Owner

Choose a reason for hiding this comment

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

Tabs

Copy link
Author

Choose a reason for hiding this comment

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

I've never used go and I'm a "spaces not tabs" person so I was unaware of the standard or that I had done it differently than yours. I ran it through gofmt, given that's the official tool I'd imagine that's your expectation. Let me know if it needs anything else.

Copy link
Owner

Choose a reason for hiding this comment

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

I am also a spaces person as well, but Go has some opinions on formatting that I like to follow.

Copy link
Author

Choose a reason for hiding this comment

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

Wholeheartedly agree -- if the language has a suggested standard, use it.

@onedr0p
Copy link
Owner

onedr0p commented May 9, 2024

Changes appear good here, but I wonder if it could be added to the logic --enable-additional-metrics flag instead of creating separate config flags / env options? I am not sure if there is a use-case for why you would want --enable-additional-metrics on but turn off the history & wanted/missing flags? The --enable-additional-metrics flag was mainly created for those with large libraries that need to turn off the slower API calls.

Also @rtrox would be great if/when you are free to review to get a pair of second eyes on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants