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-bitcoin-exporter: init #89267

Merged
merged 3 commits into from Apr 5, 2021
Merged

nixos/prometheus-bitcoin-exporter: init #89267

merged 3 commits into from Apr 5, 2021

Conversation

mmilata
Copy link
Member

@mmilata mmilata commented May 31, 2020 •
edited

Motivation for this change

I like to monitor things.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@mmilata mmilata mentioned this pull request May 31, 2020
Open
@mmilata mmilata changed the title nixos/bitcoin-prometheus-exporter: init nixos/prometheus-bitcoin-exporter: init May 31, 2020
@ofborg ofborg bot requested a review from jb55 May 31, 2020 20:59
mdlayher
mdlayher reviewed Jun 10, 2020
View reviewed changes
Copy link
Member

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

I don't know much about the Python bits but left some comments on the Prometheus aspects.

nixos/modules/services/monitoring/prometheus/exporters/bitcoin.nix
'';
};

refreshSeconds = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Typically you would want a Prometheus exporter to fetch all metrics immediately whenever queried, so that way it's up to Prometheus to decide how often the scrape interval should be. Why is this exporter configured this way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately that's how the exporter is written, to periodically poll the daemon. We can hardcode this to some low value and pretend that the option doesn't exist but I suppose it's better to not introduce such magic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened suggestion to upstream: jvstein/bitcoin-prometheus-exporter#12

nixos/tests/prometheus-exporters.nix Outdated Show resolved Hide resolved
@mmilata
Copy link
Member Author

mmilata commented Jun 10, 2020

Updated the PR to include the patch that makes listening address of the exporter configurable, since it's been merged to master.

@mmilata mmilata marked this pull request as ready for review June 22, 2020 21:57
@mmilata
Copy link
Member Author

mmilata commented Jun 22, 2020

#89247 has been merged & this is ready for review, PTAL.

@mmilata
Copy link
Member Author

mmilata commented Apr 4, 2021

Rebased to master & fixed test failure due to port number collision.

@prusnak
Copy link
Member

prusnak commented Apr 5, 2021

@risicle do you want to have a look at this one too?

@risicle
Copy link
Contributor

risicle commented Apr 5, 2021

I don't really know anything about bitcoin.

@prusnak prusnak merged commit 073fbba into NixOS:master Apr 5, 2021
@mmilata mmilata deleted the bitcoin-exporter branch April 6, 2021 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prusnak prusnak prusnak left review comments

@mdlayher mdlayher mdlayher left review comments

@jb55 jb55 Awaiting requested review from jb55

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@WilliButz WilliButz Awaiting requested review from WilliButz

Assignees
No one assigned
Labels
6.topic: nixos 6.topic: python 8.has: clean-up 8.has: module (new) 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mmilata @prusnak @risicle @mdlayher @ryantm @SuperSandro2000