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.

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

nixos/promscale: init #97372

Open
wants to merge 1 commit into
base: master
from

Conversation

@0x4A6F
Copy link
Contributor

@0x4A6F 0x4A6F commented Sep 7, 2020

Motivation for this change

Enable service for timescale-prometheus.

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.
@0x4A6F 0x4A6F force-pushed the 0x4A6F:master-timescale-prometheus-service branch 3 times, most recently from c53f3cb to 794ffe5 Sep 9, 2020
Copy link
Contributor

@aanderse aanderse left a comment

Thanks for taking care of the password issue!

I have taken the time to actually give this module a review now. I hope at least some of the comments are helpful.

'';
};

db-connect-retries = mkOption {

This comment has been minimized.

@aanderse

aanderse Sep 11, 2020
Contributor

Again, purely a style thing, feel free to ignore 100% and then some but... I think most modules typically use a database. prefix for database related options, something like

database = {
  connect-retries = mkOption { ... };
  host = mkOption { ... };
  name = mkOption { ... };
};

Consistency is always nice.

This comment has been minimized.

@0x4A6F

0x4A6F Sep 11, 2020
Author Contributor

If I'm going to stray away from original commandline argument mapping, I'd first should get rid of web-listen-address and use listenAddress and listenPort instead.

nixos/tests/timescale-prometheus.nix Outdated Show resolved Hide resolved
nixos/tests/timescale-prometheus.nix Outdated Show resolved Hide resolved
Copy link
Contributor Author

@0x4A6F 0x4A6F left a comment

Thanks for your review.

'';
};

db-connect-retries = mkOption {

This comment has been minimized.

@0x4A6F

0x4A6F Sep 11, 2020
Author Contributor

If I'm going to stray away from original commandline argument mapping, I'd first should get rid of web-listen-address and use listenAddress and listenPort instead.

@aanderse
Copy link
Contributor

@aanderse aanderse commented Sep 12, 2020

Changes are looking good. I left a few minor comments.

I should have asked before but made the assumption it cannot... so I'll ask now: can this program accept a configuration file instead of environment variables?

@0x4A6F 0x4A6F force-pushed the 0x4A6F:master-timescale-prometheus-service branch from 2bd44f8 to 1071d7c Sep 12, 2020
Copy link
Contributor

@aanderse aanderse left a comment

This looks great. Do we have any subject matter experts in the community that can give this a quick one over? From my perspective we're pretty much ready for a merge 👍

@0x4A6F
Copy link
Contributor Author

@0x4A6F 0x4A6F commented Sep 20, 2020

Thanks, MR is updated addressing your suggestions.

I should also extend services.prometheus with remote_write and remote_read. Probably in another MR #99180, when ready to merge this one.

@0x4A6F 0x4A6F force-pushed the 0x4A6F:master-timescale-prometheus-service branch from 361e946 to c395a6e Sep 20, 2020
Copy link
Contributor

@aanderse aanderse left a comment

This looks fine to me but you need someone who knows more about prometheus to review this before merge please.

@0x4A6F 0x4A6F force-pushed the 0x4A6F:master-timescale-prometheus-service branch from c395a6e to e066be5 Oct 6, 2020
@0x4A6F 0x4A6F changed the title nixos/timescale-prometheus: init nixos/promscale: init Oct 6, 2020
@0x4A6F
Copy link
Contributor Author

@0x4A6F 0x4A6F commented Oct 6, 2020

I've updated again to reflect the project rename to promscale, depends on #99875.

Thanks for your review. TIme to search for our prometheus experts.
Wondering, it this is better placed in nixos/modules/services/monitoring/prometheus/promscale.nix.

@aanderse
Copy link
Contributor

@aanderse aanderse commented Oct 6, 2020

You might consider mentioning this on one of the discourse threads.

@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 6, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/324

@0x4A6F 0x4A6F force-pushed the 0x4A6F:master-timescale-prometheus-service branch from e066be5 to ff85bdd Oct 8, 2020
# "-web-cors-origin example.com/public/.*" # corsOrigin:0xc00016dea0
];
services.postgresql = {
authentication = pkgs.lib.mkOverride 10 ''

This comment has been minimized.

@andir

andir Oct 29, 2020
Member

You can probably get rid of this by using /run/postgresql/ as "host" in this test. By default postgresql uses the unix user that connects (on NixOS)

This comment has been minimized.

@0x4A6F

0x4A6F Oct 29, 2020
Author Contributor

Doesn't seem to work without authentication set and db-host = "/run/postgresql/";.
Seems with these settings promscale is started before postgres is up.

@0x4A6F
Copy link
Contributor Author

@0x4A6F 0x4A6F commented Oct 29, 2020

Currently with promscale 0.1.1 (#102013) the test breaks, because of postgres version mismatch.
Now it needs postgresql 12, tested with following in my local branch:

--- a/pkgs/top-level/all-packages.nix
+++ b/pkgs/top-level/all-packages.nix

-  postgresql = postgresql_11.override { this = postgresql; };
+  postgresql = postgresql_12.override { this = postgresql; };
   postgresqlPackages = recurseIntoAttrs postgresql.pkgs;
   postgresql11Packages = pkgs.postgresqlPackages;
+  postgresql12Packages = pkgs.postgresqlPackages;
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.