feat: reorganize code to remove the main package#1238
Conversation
cristiangreco
force-pushed
the
cristian/rename-main-package
branch
from
cb47e7b to
8d2bfd5
Compare
| // limitations under the License. | ||
|
|
||
| package main | ||
| package exporter |
There was a problem hiding this comment.
All the code added here is just moved from main.go
cmd/main.go
Outdated
There was a problem hiding this comment.
I don't think this file should be moved out of the postgres_exporter package. That name is significant to as it becomes the default binary name.
exporter/postgres_exporter.go
Outdated
| Config: &config.Config{}, | ||
| } | ||
|
|
||
| configFile = kingpin.Flag("config.file", "Postgres exporter configuration file.").Default("postgres_exporter.yml").String() |
There was a problem hiding this comment.
I don't think that the flags make sense to move out of the main package. They're specific to the CLI for this exporter. I think it would be weird for another project to want these CLI flags as-is and with no ability to turn that off. Think of something like Grafana Alloy which I believe does import this project - this would potentially pollute their own CLI. I think that the exporter package should just make public something that can take a config or the values or some other pattern that projects can import.
exporter/postgres_exporter.go
Outdated
| } | ||
|
|
||
| func RunPostgresExporter() { | ||
| kingpin.Version(version.Print(exporterName)) |
There was a problem hiding this comment.
This would also pollute the importing projects' CLI
cristiangreco
force-pushed
the
cristian/rename-main-package
branch
7 times, most recently
from
ef0ebda to
099607d
Compare
Move all `main` code to `exporter`, for easier code embedding. Signed-off-by: Cristian Greco <cristian@regolo.cc>
cristiangreco
force-pushed
the
cristian/rename-main-package
branch
from
099607d to
d8dfe15
Compare
sysadmind
left a comment
sysadmind
left a comment
There was a problem hiding this comment.
Wow so many instances where we used a global var for the logger. I never realized that was the case.
exporter/postgres_exporter.go
Outdated
| } | ||
|
|
||
| // WithLogger configures logger. | ||
| func WithLogger(logger *slog.Logger) ExporterOpt { |
There was a problem hiding this comment.
I think this is somewhat fragile. If I'm not mistaken, this has to be before any argument that uses a logger and if you forget, other functions would just use the noop logger. That means the logs just disappear even though you had an error that was probably fatal.
What do you think about instead changing NewExporter to require the logger? You can still pass in noop, but that's an explicit decision.
There was a problem hiding this comment.
Makes sense, I've updated the NewExporter signature to include the logger in 6aab94f
cristiangreco
force-pushed
the
cristian/rename-main-package
branch
2 times, most recently
from
f684475 to
94d8cf1
Compare
Signed-off-by: Cristian Greco <cristian@regolo.cc>
cristiangreco
force-pushed
the
cristian/rename-main-package
branch
from
94d8cf1 to
6aab94f
Compare
3 participants
Move all
maincode toexporter, for easier code embedding.This is mostly based on previous work from @thampiotr at github.com/grafana/postgres_exporter