-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Update to exporter-toolkit v0.8.2 #979
Update to exporter-toolkit v0.8.2 #979
Conversation
Signed-off-by: Perry Naseck <[email protected]>
Build is failing because Please advise on the preferred solution. |
Bumping the indirect dependencies is the preferred method. For example |
Maybe we should move the external URL and route prefix features to the toolkit next. |
Signed-off-by: Perry Naseck <[email protected]>
Fixed dependency issues! Now passes tests.
Yeah, I can look into that next possibly. Though no promises as my non-work open source timeline is often sporadic. The route prefix option seems pretty popular, though I guess in my limited experience so far I have not encountered external URL as much. I think if we were to formalize getting data out of the flags then we should probably implement helper methods instead of encouraging struct access to avoid possible later breaking changes. This is more a discussion for the toolkit repo so maybe I will open an issue there. For v0.8.0 though, this should be good to go (pun intended)! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to pull in the bug fix, otherwise LGTM.
Signed-off-by: Perry Naseck <[email protected]>
Bumped exporter-toolkit to v0.8.1! |
@SuperQ just checking in on this! |
@@ -92,7 +91,14 @@ func run() int { | |||
level.Info(logger).Log("msg", "Loaded config file") | |||
|
|||
// Infer or set Blackbox exporter externalURL | |||
beURL, err := computeExternalURL(*externalURL, *listenAddress) | |||
listenAddrs := toolkitFlags.WebListenAddresses | |||
if *externalURL == "" && *toolkitFlags.WebSystemdSocket { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be *externalURL != ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it is correct, and I tested it. This if statement intentionally throws an error if an external URL is not explicitly provided when using a systemd socket listener. This is because the external URL can never be inferred from the systemd socket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, what if you don't care about the external URL? It's an optional setting in the normal case. Wouldn't this for the user to set an external URL in systemd mode? I don't think that's desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
External URL is an optional setting because it infers it from the port number or hostname provided when no external URL is provided. It can't infer this with the systemd socket. The default from the optional flag is an empty string, but then the function called on it later finds the real fallback value.
If you're saying the external URL is not actually needed for the exporter to function, then I think that goes past the scope of this pull. The existing function attempts to find a fallback default value when not is provided.
Thanks! |
This pull updates the blackbox_exporter to use the latest version of exporter-toolkit and includes the new flag methods.
Associated exporter-toolkit pull: prometheus/exporter-toolkit#95
Similar node_exporter pull: prometheus/node_exporter#2393
cc @SuperQ @roidelapluie