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

Remove prometheus version check? #1159

Closed
miekg opened this issue May 17, 2019 · 6 comments
Closed

Remove prometheus version check? #1159

miekg opened this issue May 17, 2019 · 6 comments

Comments

@miekg
Copy link
Contributor

miekg commented May 17, 2019

Hi there,

We're using thanos (0.4) but it checks the version of prometheus (I understand why this is done) on startup. Unfortunate we compile our own prometheus and add some fluff to the version. This makes the check fail and prevents thanos (store) from starting.

We've patched out the check for now.

I'm not sure what the right course of action here is. Either allow the check to fail and still start (via some cmdline flag) or some such?

A better, but more involved way, would be to probe for functionality you need want from prometheus and only abort when that is not available.

@bwplotka
Copy link
Member

bwplotka commented May 17, 2019

Thanks for reporting. Yea I think sidecar erroring on custom build of Prometheus is wrong. I am pretty sure we should make this check non-blocking OR remove version fetch totally and try to grab flags and if fails with 404 just continue without those extra checks. What do you think?

PRs welcome (:

@miekg
Copy link
Contributor Author

miekg commented May 17, 2019

Removing code is always my preferred solution :)

Just logging and continuing might just work here? I.e

  • version parsed and is OK -> continue
  • version parsed not OK -> abort
  • version not parsed -> log it -> continue

@bwplotka
Copy link
Member

It's more involved if you see the code. AFAIK we check version ONLY to double check if we can continue with extra validation -> as only from 2.2.X there is new flags endpoint (:

I would say if we can drop version check and just check flags and if 404 then log and continue without validation (treat it as below 2.2.x Prometheus)

@miekg
Copy link
Contributor Author

miekg commented May 17, 2019

Ah yes, I see in sidecar.go:

func validatePrometheus(ctx context.Context, logger log.Logger, m *promMetadata) error {
	if m.version == nil {
		level.Warn(logger).Log("msg", "fetched version is nil or invalid. Unable to know whether Prometheus supports /version endpoint, skip validation")
		return nil
	}

	if m.version.LessThan(promclient.FlagsVersion) {
		level.Warn(logger).Log("msg",
			"Prometheus doesn't support flags endpoint, skip validation", "version", m.version.Original())
		return nil
	}

And then it will try to get the flags from prometheus and perform some validation.

From the looks of it, the version stuff can removed and just try to grab the flags; if they are there do the extra validation and otherwise just log something and return nil.

This means removing the version related functions from promclient/promclient.go and related changes in cmd/thanos/sidecar.go

@miekg
Copy link
Contributor Author

miekg commented May 17, 2019

Potential PR: #1163

@GiedriusS
Copy link
Member

Closing as the PR was merged.

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

No branches or pull requests

3 participants