feat: added prober package to generalize readiness probes #1242

FUSAKLA · 2019-06-10T21:40:53Z

This PR adds prober package/component which will unify readiness and health checks of all thanos components.

For more info please refer to the PR #656

bwplotka

Awesome, thanks for splitting. This would much much faster then in review!

Some initial comments, need to run home (:

bwplotka · 2019-06-11T18:22:10Z

pkg/component/component.go

@@ -6,6 +6,11 @@ import (
 	"github.com/improbable-eng/thanos/pkg/store/storepb"
 )

+// Component is a generic component interface


missing trailing period

thanks, should be added

pkg/prober/prober.go

bwplotka · 2019-06-11T18:23:11Z

pkg/prober/prober.go

+}
+
+// SetLogger sets logger used by the Prober.
+func (p *Prober) SetLogger(logger log.Logger) {


Is it needed?

Yeah, it was needed formerly when there was mutex for logger as well but you suggested to drop it and I just didn't notice there is no need for the getter/setter now without it.
Thanks for pointing out, will remove.

bwplotka · 2019-06-11T18:23:16Z

pkg/prober/prober.go

+	p.logger = logger
+}
+
+func (p *Prober) getLogger() log.Logger {


same here, do we need this?

removed, more info above

bwplotka · 2019-06-11T18:25:13Z

pkg/prober/prober.go

+func NewProber(component component.Component, logger log.Logger) *Prober {
+	initialErr := fmt.Errorf(initialErrorText, component)
+	prober := &Prober{}
+	prober.SetComponent(component)


Prober should not use public method TBH (:

Can we just create prober with filled struct?

Definitely, just out of curiosity why should be this avoided? Is that generic thing in Golang not to use own public methods?

I removed also calls to SetNotReady and SetNotHealthy which caused that the mutex locking is gone but since it's the instantiation of the struct it should be ok I hope?

Because there is no need for this. Literally we will never in our code require dynamic reload of component. We don't have components that transforms into another components right?

Every bit of genercity is introducing complexity. Since we don't need that now (YAGNI rule), we can avoid that complexity, right? (:

Makes sense, thanks for for the explanation :)

FUSAKLA · 2019-06-11T21:16:43Z

@bwplotka thanks fot the comments! Should be all resolved now I hope.

bwplotka

Better (:

Still some comments! Let's aim for perfection.

bwplotka · 2019-06-14T15:41:38Z

pkg/prober/prober.go

+// NewProber returns Prober representing readiness and healthiness of given component.
+func NewProber(component component.Component, logger log.Logger) *Prober {
+	initialErr := fmt.Errorf(initialErrorFmt, component)
+	prober := &Prober{


you don't need this variable, IMO just return &Prober

bwplotka · 2019-06-14T15:41:53Z

pkg/prober/prober.go

+	return prober
+}
+
+// RegisterInRouter registers readiness and liveness probes to mux.


Suggested change

// RegisterInRouter registers readiness and liveness probes to mux.

// RegisterInRouter registers readiness and liveness probes to router.

bwplotka · 2019-06-14T15:43:07Z

pkg/prober/prober.go

+
+// IsReady returns error if component is not ready and nil if it is.
+func (p *Prober) IsReady() error {
+	p.readyMtx.Lock()


Let's Move to sync.RWMutex and do Rlock and RUnlock here (: There will be more readers than writerrs.

Great, thanks I didn't know there is also RWMutex, TIL :)

bwplotka · 2019-06-14T15:44:42Z

pkg/prober/prober.go

+	}
+}
+
+// IsReady returns error if component is not ready and nil if it is.


Suggested change

// IsReady returns error if component is not ready and nil if it is.

// IsReady returns error if component is not ready and nil otherwise

bwplotka · 2019-06-14T15:46:02Z

pkg/prober/prober.go

+
+// SetReady sets components status to ready.
+func (p *Prober) SetReady() {
+	if p.IsReady() != nil {


This is wrong I think as those two methods are locked so you can have a race. IF you need this log line, please add this to SetNotReady

Ok, just so I understand where the race is. The IsReady locks but once finished it again unlocks it. And only after it returns and it does not return nil the SetNotReady is called which again locks and unlocks. Which part could go wrong here?

Sorry, now I get it. Meanwhile I unlock it the status of prober could change.

bwplotka · 2019-06-14T15:47:06Z

pkg/prober/prober.go

+
+// SetNotReady sets components status to not ready with given error as a cause.
+func (p *Prober) SetNotReady(err error) {
+	if err != nil && p.IsReady() == nil {


This is wrong from the same reason above, there can be a race.

Please lock here and then check p.readiness instead to check what to log. What do you think? (:

bwplotka · 2019-06-14T15:47:56Z

pkg/prober/prober.go

+
+// SetHealthy sets components status to healthy.
+func (p *Prober) SetHealthy() {
+	if p.IsHealthy() != nil {


Exactly the same issue as for Readiness.

Let's move to RWLock and log under lock as well (it's only on write so should be fine)

bwplotka · 2019-06-14T15:48:31Z

pkg/prober/prober.go

+// HandleIfReady if probe is ready calls the function otherwise returns 503.
+func (p *Prober) HandleIfReady(f http.HandlerFunc) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		p.readyMtx.Lock()


Why not reusing p.IsReady? (: In this case it's fine.

Especially that we don't know how long f is and what it does from this function perspecitve.

Good catch. This could cause serious issues.

FUSAKLA · 2019-06-16T18:58:25Z

@bwplotka thank you for all the valuable comments/suggestions and for answering all my stupid questions so I can learn from it, I really appreciate it.

Is it ok with you this way?

bwplotka · 2019-06-20T13:47:58Z

No worries, just tell if you are rdy for next review iteration (: I think it's rdy now?

FUSAKLA · 2019-06-20T14:27:56Z

Yes, it should be ready I hope.

FUSAKLA · 2019-07-01T18:41:41Z

@bwplotka if you find a while and could take a look at this?

It's rebased and I added metrics for observing the Prober state if that is ok with you

thanos_<component>_ready
thanos_<component>_healthy (wasn't sure about this one since it should be 1 whenever e serve metrics but on the other hand it felt weird not to expose it either)

bwplotka

@FUSAKLA 🎉 Amazing code right now 👍

Super clean and with attention to details - I love it.

I have some more suggestions let me know what you think. Otherwise LGTM! Thanks.

bwplotka · 2019-07-01T20:20:44Z

pkg/prober/prober.go

+const (
+	healthyEndpointPath  = "/-/healthy"
+	readyEndpointPath    = "/-/ready"
+	okProbeFmt           = "thanos %v is %v"


Can we inline all constants that are used only once? (: There is no point of those if they are not references more than once.

yes, inlined them probably will be easier to read 👍

bwplotka · 2019-07-01T20:21:30Z

pkg/prober/prober.go

+}
+
+func (p *Prober) writeResponse(w http.ResponseWriter, probeFunc func() error, probeType string) {
+	err := probeFunc()


you can do if err := probeFn(); err != nil here (: Also we usually do Fn suffix for function vars so probeFn would be nice? (: What do you think?

bwplotka · 2019-07-01T20:21:55Z

pkg/prober/prober.go

+		http.Error(w, fmt.Sprintf(errorProbeFmt, p.component, probeType, err), probeErrorHTTPStatus)
+		return
+	}
+	if _, e := io.WriteString(w, fmt.Sprintf(okProbeFmt, p.component, probeType)); e == nil {


please let's keep err as error variable name! (:

Becuse of introducing the new name there is a bug here -> you log "err", err not "err", e as it should be (:

thanks for noticing, changed to err

bwplotka · 2019-07-01T20:32:28Z

pkg/prober/prober.go

+		healthiness: initialErr,
+		readiness:   initialErr,
+		readyStateMetric: prometheus.NewGauge(prometheus.GaugeOpts{
+			Name: fmt.Sprintf("thanos_%s_ready", component),


TBH instead of that I would just do `prober_ready", and then:

have label component. This will allow for better, potential aggregations in future.

you can have just prober_ready and do not specify thanos prefix. This allows to reuse this package outside of Thanos even (why not)!

Thanos can then on caller side do prometheus.WrapRegistererWithPrefix("thanos" (:

What do you think?

same for prober_healthy

I was thinking about the naming a lot bud than looked to the rest of packages and found everywhere hardcoded the thanos prefix so I wanted to stay consistent but I agree your suggestion is more variable.

Applied as suggested

Yes, we did not think about that in initial implementation, but now I would aim for no thanos_ prefix inside packages.

Ideally only main adds those

bwplotka · 2019-07-01T20:33:30Z

BTW nice idea with metric 👍

FUSAKLA · 2019-07-01T21:17:44Z

@bwplotka thanks for those comments! All should be resolved as you suggested.

bwplotka

Awesome! Thanks for this 👍

FUSAKLA force-pushed the fus-add-readiness-prober branch from cb9b27f to a58e510 Compare June 10, 2019 22:13

bwplotka requested changes Jun 11, 2019

View reviewed changes

FUSAKLA force-pushed the fus-add-readiness-prober branch from b054f62 to c2961f1 Compare June 11, 2019 20:46

bwplotka requested changes Jun 14, 2019

View reviewed changes

FUSAKLA force-pushed the fus-add-readiness-prober branch from c2961f1 to f4c6453 Compare June 16, 2019 18:52

FUSAKLA added 3 commits July 1, 2019 19:42

feat: added prober package to generalize readiness probes

a650a39

fix prober: minor fixes after CR

cf32611

fix prober: refactored mutex locking

06ed8e1

FUSAKLA force-pushed the fus-add-readiness-prober branch from f4c6453 to 06ed8e1 Compare July 1, 2019 17:43

feat prober: added prober state status metrics

7c4c1cb

bwplotka approved these changes Jul 1, 2019

View reviewed changes

refactor prober: minor changes in code style

ce599e6

bwplotka approved these changes Jul 1, 2019

View reviewed changes

bwplotka merged commit 3d30a90 into thanos-io:master Jul 1, 2019

	// RegisterInRouter registers readiness and liveness probes to mux.
	// RegisterInRouter registers readiness and liveness probes to router.

	// IsReady returns error if component is not ready and nil if it is.
	// IsReady returns error if component is not ready and nil otherwise

feat: added prober package to generalize readiness probes #1242

feat: added prober package to generalize readiness probes #1242

Conversation

FUSAKLA commented Jun 10, 2019

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

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

FUSAKLA commented Jun 11, 2019

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

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

FUSAKLA commented Jun 16, 2019

bwplotka commented Jun 20, 2019

FUSAKLA commented Jun 20, 2019

FUSAKLA commented Jul 1, 2019 • edited Loading

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

bwplotka Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

bwplotka commented Jul 1, 2019

FUSAKLA commented Jul 1, 2019

bwplotka left a comment

Choose a reason for hiding this comment

FUSAKLA commented Jul 1, 2019 •

edited

Loading

bwplotka Jul 1, 2019 •

edited

Loading