-
Notifications
You must be signed in to change notification settings - Fork 738
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
skip SPACE_TYPE column for MariaDB >=10.5 #724
base: main
Are you sure you want to change the base?
Conversation
- to be able to scrape information_schema.innodb_sys_tablespaces - https://jira.mariadb.org/browse/MDEV-19940 Signed-off-by: Birk Bohne <birk.bohne@sap.com>
- to avoid panic errors with invalid version numbers - unit test added Signed-off-by: Birk Bohne <birk.bohne@sap.com>
- to be able to validate code with and without SPACE_TYPE column - version query skipped if running inside test Signed-off-by: Birk Bohne <birk.bohne@sap.com>
@businessbean Can I do anything to help get this into main? |
Hi @firecow , you should filter these logs already on Logstash level 😄 I will update the PR today... |
@businessbean Nice, thanks man.. |
@@ -72,7 +72,7 @@ func (ScrapeBinlogSize) Version() float64 { | |||
} | |||
|
|||
// Scrape collects data from database connection and sends it over channel as prometheus metric. | |||
func (ScrapeBinlogSize) Scrape(ctx context.Context, db *sql.DB, ch chan<- prometheus.Metric, logger log.Logger) error { | |||
func (ScrapeBinlogSize) Scrape(ctx context.Context, db *sql.DB, ch chan<- prometheus.Metric, logger log.Logger, semanticVersionIsNewer bool) error { |
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.
This semanticVersionIsNewer
is not a good way to handle this. It's too specific to one collector, and hard-codes it as a flag for every collector.
What we probably want is to do something like we did in the postgres_expoter, which is to make the db
instance include both a database connection and a semver that each collector can access.
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.
Do you have a link to the related code?
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.
Here's the PR: prometheus-community/postgres_exporter#785
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.
I would prefer to do that refactoring as a separate PR.
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.
I will look into this, but it will take some time...
Signed-off-by: Birk Bohne <birk.bohne@sap.com>
is there any chance to merge it? 🙂 or is it waiting for the refactor? can I help with some testing, rebasing or coding? It's not exactly clear to me what blocked the merge 🙂 |
hello @SuperQ are you still working on this repo? It would be useful for us to merge it. I can do some changes if needed. We are currently making custom build for this library because of this |
Hi @SuperQ,
this PR skips the
SPACE_TYPE
column for MariaDB server instances with version10.5
or newer.information_schema.innodb_sys_tablespaces
tablesbuild environment
make build
unit test results
MariaDB test results
10.4.28
10.5.18