promclient: drop version check #1163

miekg · 2019-05-17T13:40:52Z

The version check would check the prometheus version and if > 2.2.0
would query the flags endpoint to do some extra validation.

Remove this for just checking the flags endpoints and if it responds do
the validation; otherwise log the failure, but continue the startup.

Signed-off-by: Miek Gieben miek@miek.nl

Changes

Verification

bwplotka

Overall good - as we agreed on issue, but we some suggestions to make it work.

cmd/thanos/sidecar.go

bwplotka · 2019-05-17T13:43:44Z

cmd/thanos/sidecar.go

-			"Prometheus doesn't support flags endpoint, skip validation", "version", m.version.Original())
-		return nil
-	}
-
 	flags, err := promclient.ConfiguredFlags(ctx, logger, m.promURL)


Can we add flow for distincting 404 from that will show that either it's not Prometheus (this will be caught later on, so fine). Maybe via simple sentinel erorr?

I am afraid this has to retry ): As we removed first retry which helped us to wait until Prometheus is rdy (often the case). Thanks of first retry you removed we wouldn't need to retry here, as we know Prometheus must be rdy. This is not the case now without retry you removed ):

Let me know if my response is clear (:

yes. The 404 is a bit fiddly, because we see this. But PTAL

bwplotka · 2019-05-17T18:50:56Z

pkg/promclient/promclient.go

+		}
+
+		if resp.StatusCode == 200 || resp.StatusCode == 404 { // 404, just give up
+			return nil


I would say something like

var NotFoundFlags = errors.New("No flag endpoint found")

has to be returned here. And let caller decide what to do -> e.g we should log on 404 and, grab flags on 200 and error on anything else IMO (:

Sorry for being strict ):

But I'm not sure how you want to do that with the retry logic? If I understand that code correctly it retries upon seeing an error, meaning it will retry NotFoundFlags?

Well we need to pass it somehow. This question is just implementation detail. We either move retry logic to caller (I prefer that - more explicit) and just retry on certain errors or just pass error in separate var here.

Ok, moved and added a few things back to not make too many changes.

povilasv · 2019-05-21T06:27:00Z

I think maybe we should keep the check but make the check to check for 2.x.
As this protects users from running super old Prometheus versions (1.x) and future versions.

bwplotka · 2019-05-21T07:45:54Z

@povilasv we cannot. As users can build prom on their own so they can put whatever in a version. If they use promu and build from master it will have some commit + date + something version. Let's drop version check and try flags instead

miekg · 2019-05-21T08:55:30Z

[ Quoting <notifications@github.com> in "Re: [improbable-eng/thanos] promcli..." ]

bwplotka commented on this pull request. > - return Flags{}, errors.Errorf("got non-200 response code: %v, response: %v", resp.StatusCode, string(b)) + if err := runutil.Retry(2*time.Second, ctx.Done(), func() error { + + resp, err := http.DefaultClient.Do(req.WithContext(ctx)) + if err != nil { + return errors.Wrapf(err, "request config against %s", u.String()) + } + defer runutil.CloseWithLogOnErr(logger, resp.Body, "query body") + + body, err = ioutil.ReadAll(resp.Body) + if err != nil { + return errors.New("failed to read body") + } + + if resp.StatusCode == 200 || resp.StatusCode == 404 { // 404, just give up + return nil Well we need to pass it somehow. This question is just implementation detail. We either move retry logic to caller (I prefer that - more explicit) and just retry on certain errors or just pass error in separate var here.

sgtm. To be clear, basically move the runutil.Retry to whatever is calling this right now (don't have the code open atm)

The version check would check the prometheus version and if > 2.2.0 would query the flags endpoint to do some extra validation. Remove this for just checking the flags endpoints and if it responds do the validation; otherwise log the failure, but continue the startup. Signed-off-by: Miek Gieben <miek@miek.nl>

povilasv

A couple of small knits, other than that LGTM 🥇

pkg/promclient/promclient.go

Signed-off-by: Miek Gieben <miek@miek.nl>

miekg · 2019-05-21T10:33:40Z

ok, done.

povilasv

👍 @bwplotka take another look when you have time.

bwplotka

LGTM, but still something we could improve. WDYT? @miekg

bwplotka · 2019-05-22T12:54:28Z

cmd/thanos/sidecar.go

-	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
+	flags := promclient.Flags{


This works, but it might be bit confusing - it's not really following the logical path, which can surprise reader. If there is no flags the whole function should just return. We workaround simple noFlagEndpoint bool with assuming some values and unneccesary validating them below - prone to bug in future. But not a blocker, we can merge this and fix it later.

miekg · 2019-05-22T12:59:20Z

Happy to provide further fixes l, after potential merge. I'm still wondering about the whole mechanism, as you can reload prom configs on the fly...

…

On Wed, 22 May 2019, 13:56 Bartek Płotka, ***@***.***> wrote: ***@***.**** approved this pull request. LGTM, but still something we could improve. WDYT? @miekg <https://github.com/miekg> ------------------------------ In cmd/thanos/sidecar.go <#1163 (comment)> : > @@ -321,20 +305,27 @@ func runSidecar( } 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 + flags := promclient.Flags{ This works, but it might be bit confusing - it's not really following the logical path, which can surprise reader. If there is no flags the whole function should just return. We workaround simple noFlagEndpoint bool with assuming some values and unneccesary validating them below - prone to bug in future. But not a blocker, we can merge this and fix it later. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1163>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACWIW2NQTGGNVQ5BMZ66I3PWU7I7ANCNFSM4HNVMDOA> .

GiedriusS · 2019-05-22T13:12:33Z

Happy to provide further fixes l, after potential merge. I'm still wondering about the whole mechanism, as you can reload prom configs on the fly...
…
On Wed, 22 May 2019, 13:56 Bartek Płotka, @.> wrote: @.* approved this pull request. LGTM, but still something we could improve. WDYT? @miekg https://github.com/miekg ------------------------------ In cmd/thanos/sidecar.go <#1163 (comment)> : > @@ -321,20 +305,27 @@ func runSidecar( } 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 + flags := promclient.Flags{ This works, but it might be bit confusing - it's not really following the logical path, which can surprise reader. If there is no flags the whole function should just return. We workaround simple noFlagEndpoint bool with assuming some values and unneccesary validating them below - prone to bug in future. But not a blocker, we can merge this and fix it later. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1163?email_source=notifications&email_token=AACWIW6PRYQKQ2DCZUD5PBTPWU7I7A5CNFSM4HNVMDOKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOBZLWQVY#pullrequestreview-240609367>, or mute the thread https://github.com/notifications/unsubscribe-auth/AACWIW2NQTGGNVQ5BMZ66I3PWU7I7ANCNFSM4HNVMDOA .

Is there really a problem with it? Min/max block duration in Prometheus are configured via command line options and AFAIK you can't change them without restarting the whole process, and if you are going to restart the whole process then Sidecar will get "disconnected" from Prometheus and it will recheck those again after getting "reconnected". Have I missed something? 🤔

miekg · 2019-05-22T16:24:37Z

ah, I wasn't aware it's a command line flag; that makes more sense then 👍

bwplotka

This works, but it might be bit confusing - it's not really following the logical path, which can surprise reader. If there is no flags the whole function should just return. We workaround simple noFlagEndpoint bool with assuming some values and unneccesary validating them below - prone to bug in future. But not a blocker, we can merge this and fix it later.

This needs to be fixed at some point, but let's merge this to unblock custom builds 👍
Thanks @miekg !

bwplotka requested review from povilasv and GiedriusS May 17, 2019 13:41

miekg mentioned this pull request May 17, 2019

Remove prometheus version check? #1159

Closed

bwplotka self-requested a review May 17, 2019 13:41

bwplotka reviewed May 17, 2019

View reviewed changes

povilasv approved these changes May 21, 2019

View reviewed changes

pkg/promclient/promclient.go Outdated Show resolved Hide resolved

pkg/promclient/promclient.go Outdated Show resolved Hide resolved

address nits from review

c01cff8

Signed-off-by: Miek Gieben <miek@miek.nl>

povilasv approved these changes May 21, 2019

View reviewed changes

povilasv requested a review from bwplotka May 21, 2019 10:34

bwplotka approved these changes May 22, 2019

View reviewed changes

bwplotka approved these changes May 27, 2019

View reviewed changes

bwplotka merged commit 43102df into thanos-io:master May 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

promclient: drop version check #1163

promclient: drop version check #1163

miekg commented May 17, 2019

bwplotka left a comment

bwplotka May 17, 2019

bwplotka May 17, 2019

bwplotka May 17, 2019

miekg May 17, 2019

bwplotka May 17, 2019

bwplotka May 17, 2019

miekg May 20, 2019

bwplotka May 21, 2019

miekg May 21, 2019

povilasv commented May 21, 2019

bwplotka commented May 21, 2019

miekg commented May 21, 2019 via email

povilasv left a comment

miekg commented May 21, 2019

povilasv left a comment •

edited

Loading

bwplotka left a comment

bwplotka May 22, 2019

miekg commented May 22, 2019 via email

GiedriusS commented May 22, 2019

miekg commented May 22, 2019

bwplotka left a comment

promclient: drop version check #1163

promclient: drop version check #1163

Conversation

miekg commented May 17, 2019

Changes

Verification

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

povilasv commented May 21, 2019

bwplotka commented May 21, 2019

miekg commented May 21, 2019 via email

povilasv left a comment

Choose a reason for hiding this comment

miekg commented May 21, 2019

povilasv left a comment • edited Loading

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miekg commented May 22, 2019 via email

GiedriusS commented May 22, 2019

miekg commented May 22, 2019

bwplotka left a comment

Choose a reason for hiding this comment

povilasv left a comment •

edited

Loading