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.

Sign up for GitHub

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

Jump to bottom

nixos/prometheus-exporters: rewrite and restructure #36909

Merged
merged 3 commits into from Mar 22, 2018
Merged

nixos/prometheus-exporters: rewrite and restructure #36909

merged 3 commits into from Mar 22, 2018

Conversation

WilliButz
Copy link
Member

Motivation for this change

Right now we have a single module for every prometheus-exporter (except for the ones that don't have one at all), each of them has basically the same content (except for exporter specific options).
Adding an option to all exporters or changing something that should affect all of them would require a change in each of these modules.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • I tested the basic functionality of the services.
  • Fits CONTRIBUTING.md.

cc/ @fpletz @vcunat @grahamc @globin

Thank's to @fpletz, @Ma27, @ciil, @grahamc and @globin for the feedback :)

grahamc
grahamc reviewed Mar 13, 2018
View reviewed changes
nixos/modules/services/monitoring/prometheus/exporters/minio.nix Outdated

minioAccessKey = mkOption {
type = types.str;
example = "BKIKJAA5BMMU2RHO6IBB";
Copy link
Member

Choose a reason for hiding this comment

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

Please use something that looks ... less ... real ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah sure :)
(I kept this from the original exporter module)

grahamc
grahamc reviewed Mar 13, 2018
View reviewed changes
nixos/modules/services/monitoring/prometheus/exporters/minio.nix Outdated
and <literal>config.services.minio.accessKey</literal>.
'';
} // optionalAttrs config.services.minio.enable {
default = config.services.minio.accessKey;
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be set later in the config = ... bit instead, to avoid regenerating documentation.

grahamc
grahamc reviewed Mar 13, 2018
View reviewed changes
nixos/modules/services/monitoring/prometheus/exporters/minio.nix Outdated
and <literal>config.services.minio.secretKey</literal>.
'';
} // optionalAttrs config.services.minio.enable {
default = config.services.minio.secretKey;
Copy link
Member

Choose a reason for hiding this comment

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

same as above ^

grahamc
grahamc reviewed Mar 13, 2018
View reviewed changes
nixos/tests/exporters.nix Outdated
"node"
"snmp"
# "unifi" # FIXME
# "varnish" # FIXME
Copy link
Member

Choose a reason for hiding this comment

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

What is wrong with these FIXMEs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh no, I forgot to take out the test %)
I was just using it for myself.
(These exporters fail because they can't scrape anything in this VM)

@grahamc
Copy link
Member

grahamc commented Mar 13, 2018

I like the approach a lot. Can you document what it takes for a user to define their own exporter using this tooling?

@WilliButz WilliButz force-pushed the prometheus-exporters branch 2 times, most recently from ab1a030 to 0cef4e2 Compare March 13, 2018 15:21
@WilliButz
Copy link
Member Author

WilliButz commented Mar 13, 2018 •
edited

@grahamc Where do you think the documentation about how to add another exporter should go? I'm not sure if the manual is the right place or the nixos.wiki. Maybe even both?

Nevermind, @globin just told me that there is meta.doc for modules :)

@WilliButz
Copy link
Member Author

@grahamc I added a postfix exporter module and wrote some documentation about it.

@vcunat
Copy link
Member

vcunat commented Mar 14, 2018

You edited release notes for 18.03, so you want it to be cherry-picked into there?

@WilliButz
Copy link
Member Author

@vcunat Yes I would like to see it in there, but only if that's possible :)

This was referenced Mar 20, 2018
Merged
Merged
@WilliButz
nixos/prometheus-exporters: rewrite and restructure
f4d03b5 
- prometheus exporters are now configured with
  `services.prometheus.exporters.<name>`
- the exporters are now defined by attribute sets
  from which the options for each exporter are generated
- most of the exporter definitions are used unchanged,
  except for some changes that should't have any impact
  on the functionality.
@WilliButz
release notes: add information about prometheus exporters
8a13101
@WilliButz
nixos/prometheus-exporters: add postfix exporter & documentation
c54aa1f
@globin globin merged commit 60a6c63 into NixOS:master Mar 22, 2018
@WilliButz WilliButz deleted the prometheus-exporters branch March 22, 2018 14:25
@Ma27
Copy link
Member

Ma27 commented Mar 22, 2018

awesome! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grahamc grahamc grahamc left review comments

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@WilliButz @grahamc @vcunat @Ma27 @globin @GrahamcOfBorg