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

prometheus: Split options listenAddress and port #87700

Merged
merged 3 commits into from Aug 23, 2020

Conversation

mkaito
Copy link
Contributor

@mkaito mkaito commented May 12, 2020

Motivation for this change

Accessing the configured port of a service is quite useful, for example when configuring virtual hosts for a service. The prometheus module did not expose the configured por separately, making it unnecessarily cumbersome to consume.

Things done

The option listenAddress currently has the format host:port. I've split this into two options listenAddress for the host, and port for the port, and updated the defaults.

This is a breaking change only if you were setting listenAddress to a non-standard value. If you were, you should now set listenAddress and port separately.

Perhaps there should be an assertion that listenAddress does not end in :[0-9]+$ or similar, but I'm honestly not sure how to implement that.

  • 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.

Accessing the configured port of a service is quite useful, for example
when configuring virtual hosts for a service. The prometheus module did
not expose the configured por separately, making it unnecessarily
cumbersome to consume.

This is a breaking change only if you were setting `listenAddress` to
a non-standard value. If you were, you should now set `listenAddress`
and `port` separately.
@mkaito mkaito force-pushed the mkaito/upstream/prometheus-port branch from 5f658a8 to ba3c3de Compare May 19, 2020 10:48
@Lassulus
Copy link
Member

Lassulus commented Jun 2, 2020

maybe this could be a good check?
builtins.match ".*:.*" "127.0.0.1:1234" == null

@kirelagin
Copy link
Member

Full disclosure: I haven’t tested this yet.

@kirelagin
Copy link
Member

prometheus, prometheus.passthru.tests on x86_64-linux — Success

Actually, I think, I am going to assume that everything is okay 🙄.

@Lassulus
Copy link
Member

the prometheus test fails for me, but could be unrelated:

...
store: must succeed: thanos bucket ls --objstore.config-file=/nix/store/mcprpfh8v6nwdszdivsjdg7svcyrsb2j-objstore.yaml --output=json | jq .thanos.labels.some_label | grep 'required by thanos'
store # [   54.409730] thanos[723]: level=info ts=2020-08-22T11:17:39.875663834Z caller=fetcher.go:452 component=block.BaseFetcher msg="successfully synchronized block metadata" duration=36.664437ms cached=5 returned=5 partial=0
store # Error parsing commandline arguments: expected command but got "bucket"
store # thanos: error: expected command but got "bucket"
store: output:
error:
Traceback (most recent call last):
  File "/nix/store/4b045jb1sipmsng54lz7xcf41bw3ni10-nixos-test-driver/bin/.nixos-test-driver-wrapped", line 894, in run_tests
    exec(tests, globals())
  File "<string>", line 1, in <module>
  File "<string>", line 64, in <module>
  File "/nix/store/4b045jb1sipmsng54lz7xcf41bw3ni10-nixos-test-driver/bin/.nixos-test-driver-wrapped", line 421, in succeed
    raise Exception(
Exception: command `thanos bucket ls --objstore.config-file=/nix/store/mcprpfh8v6nwdszdivsjdg7svcyrsb2j-objstore.yaml --output=json | jq .thanos.labels.some_label | grep 'required by thanos'` failed (exit code 1)
cleaning up

@Ma27
Copy link
Member

Ma27 commented Aug 22, 2020

@Lassulus the failing test should be fixed in #95845.

@Lassulus Lassulus merged commit bfd7069 into NixOS:master Aug 23, 2020
@lschuermann
Copy link
Member

Unfortunately this breaks my existing working IPv6 setup due to a colon being in the address. Something like this really shouldn't occur on 2020. 😢

Could someone please fix this?

@Lassulus
Copy link
Member

I guess this regex could be used then (didn't think about ipv6 last time sadly): "(.*\\..*):(.*)" which will match only if the address contains a dot?

@kirelagin
Copy link
Member

kirelagin commented Sep 20, 2020

Oh damn, I’m so sorry :/. Why do IP addresses have to be so complicated...

I think something like this would to the trick: "(.*\\..*|.*\\]):(.*)" – either something with a dot in it (an IPv4 address) or something ending in ] (an IPv6 address) followed by a colon and a port number.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants