-
Notifications
You must be signed in to change notification settings - Fork 769
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
Transfer code to parent repository #1
Conversation
./mysqld_exporter <flags> | ||
|
||
## Configuration | ||
The configuration is in JSON. An example with all possible options: |
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 think it might be better to be configurable entirely via flags rather than having to mess with json, there's not many possible options.
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.
IMHO this is bad idea show login/password in process list.
But if you think this is needed - I will.
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.
Hm, I'd have to agree for the credentials. Those should usually be in a config file or passed via an environment variable. @bjk-soundcloud @matthiasr any opinion on this?
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.
Yeah, password would be a problem then.
Can we make the JSON structure simpler? I think config can be removed
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.
Using env variables and then simply os.Getenv(...)
could even be nicer. It doesn't require putting another file in the filesystem, finding it, etc. Your run script can simply set an env variable, no matter where the contents of that come from.
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 agree, config
is extraneous.
I think the configuration file is fine for now. It won't be a big deal to add other ways later if desired.
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 drop config file and parse env vars
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.
+1 to ENV that contains the DSN0
This generally looks good, it'll be great to get this in. You should add an AUTHORS.md and CONTRIBUTING.md |
For reference, the ganglia0 mysqld python module is a good example of what can/needs to be scraped. |
The MySQL documentation0 has some good information on the variables in |
@@ -0,0 +1,5 @@ | |||
Exporter for mysql daemon |
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.
For repos within the prometheus
org we'd like to be consistent with NOTICE, AUTHORS.md, etc. files. Could you make this:
Exporter for MySQL daemon.
Copyright 2015 The Prometheus Authors
The best place for initial authorship attribution would be in the AUTHORS.md file, like in this: https://github.com/prometheus/node_exporter/blob/master/AUTHORS.md
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.
done
@brian-brazil show slave status may be added in future. we are don't use it in our environment, so do not implement it in first version |
|
||
rows, err := db.Query("SHOW STATUS") | ||
if err != nil { | ||
log.Printf("error running query on db") |
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'd also log the error message:
log.Println("error running status query on database:", err)
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.
ok
@eugenechertikhin Heya, what's the state on this? Are you interested in resolving the remaining bits, or should we take it from here? |
Hey Julius, |
@eugenechertikhin Cool, no problem. |
No description provided.