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

Cleanup/add gitlab exporters #819

Conversation

Sticksman
Copy link
Contributor

@Sticksman Sticksman commented Jun 23, 2023

This adds most of the diagonistic queries found in the gitlab queries.yaml: https://gitlab.com/gitlab-cookbooks/gitlab-exporters/-/blob/master/templates/postgres_exporter/queries.yaml.erb#L866

Several are difficult to write tests for:

  • pg_xid has an Nan comparison that can't be made, so had to special case that test
  • the summary queries use some pretty complex regexes that are difficult to sanitize, which means tests were nearly impossible to write. I checked on a postgres db and they seemed to work alright.

@Sticksman Sticksman force-pushed the cleanup/add-gitlab-exporters branch from e024871 to 99056e4 Compare June 23, 2023 17:35
@sysadmind
Copy link
Contributor

I know this is still a draft and changes may still come. That said I took a brief look and I have 2 notes.

  1. All of these new collectors are enabled by default. I'm not sure that every user would appreciate that behavior out of the box. The more queries that run out of the box, the higher the overhead of running this collector.
  2. I don't understand the term marginalia or why it is used.

@SuperQ
Copy link
Contributor

SuperQ commented Jun 23, 2023

+1, we should be careful about what new collectors we enable by default.

@Sticksman Sticksman force-pushed the cleanup/add-gitlab-exporters branch from be28220 to ba62ad1 Compare June 23, 2023 18:25
@Sticksman
Copy link
Contributor Author

Disabled everything by default and renamed marginalia (yeah I was kind of unclear on this term as well, I think summary was the best way I could read it as)

@Sticksman Sticksman marked this pull request as ready for review June 23, 2023 18:50
@Sticksman Sticksman force-pushed the cleanup/add-gitlab-exporters branch from a1d71b6 to 8f1a3e0 Compare June 24, 2023 20:01
@Sticksman
Copy link
Contributor Author

Will require #823

@igorwwwwwwwwwwwwwwwwwwww

I don't understand the term marginalia or why it is used.

It's from the marginalia gem.

@Sticksman
Copy link
Contributor Author

@sysadmind @SuperQ this is ready to go

@Sticksman Sticksman force-pushed the cleanup/add-gitlab-exporters branch from 11589b4 to 64bb2b5 Compare June 27, 2023 16:19
Copy link
Contributor

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Good so far. While we're importing these features, we should cleanup some best practices issues so we don't have to deal with them later.

collector/pg_long_running_transactions.go Outdated Show resolved Hide resolved
collector/pg_archiver.go Outdated Show resolved Hide resolved
collector/pg_database_wraparound.go Outdated Show resolved Hide resolved
collector/pg_index_size.go Outdated Show resolved Hide resolved
collector/pg_stat_activity_autovacuum.go Show resolved Hide resolved
collector/pg_stat_activity_autovacuum_active.go Outdated Show resolved Hide resolved
collector/pg_stat_activity_summary.go Show resolved Hide resolved
collector/pg_stat_user_indexes.go Outdated Show resolved Hide resolved
collector/pg_stat_user_indexes.go Outdated Show resolved Hide resolved
collector/pg_stat_user_indexes.go Outdated Show resolved Hide resolved
@SuperQ
Copy link
Contributor

SuperQ commented Jun 28, 2023

Honestly, I would prefer to break this up into a few PRs. Doing this all as one big PR is hard to review.

@Sticksman
Copy link
Contributor Author

I’ll work on splitting it up today

@sysadmind
Copy link
Contributor

+1 I much prefer small PRs. Much easier to review. I would suggest one PR per "collector"

Sticksman added 15 commits June 28, 2023 10:22
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
Signed-off-by: Felix Yuan <felix.yuan@reddit.com>
@Sticksman Sticksman force-pushed the cleanup/add-gitlab-exporters branch from bdf309a to 57ea9d8 Compare June 28, 2023 17:22
@Sticksman
Copy link
Contributor Author

Closing for now

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.

4 participants