Introduce concurrency limit for GET requests and a general timeout for HTTP #1743

beorn7 · 2019-02-06T17:43:50Z

Fixes #1723 .

beorn7 · 2019-02-06T17:54:01Z

@mxinden I'm banging my head against a wall when investigating the Travis errors: cannot find package "github.com/prometheus/alertmanager/ui/app" Hmm, yes, that package does not exist. But why does it even want it? It's not referred anywhere I can see. Also, it works locally.

beorn7 · 2019-02-06T18:03:17Z

I have a lead: the go generate command does not use Go Modules in my local run because apparently the env variables don't make it through to it. However, in Travis, presumably GOPATH is not set, so Go Modules are activated automatically.

beorn7 · 2019-02-06T18:12:08Z

That's definitely the reason. Now trying to find a solution. (BTW: This would fail in master in the same way.)

beorn7 · 2019-02-06T18:30:57Z

Still banging my head against a wall. The way to reproduce: set GOPATH to something else, and then run make assets. I tried to make the assets package a go module on its own, but then we get double-imports. @simonpasquier as our local go module expert: Do you know a way out of this?

api/api.go

beorn7 · 2019-02-06T18:48:01Z

Hmm, I just pushed a whitespace change to a branch freshly branched off master, and there the go generate works, but then errcheck finds a lot of problems all of a sudden.

mxinden · 2019-02-06T18:52:32Z

Interesting. Ignoring err on Log should be excluded. Will look further into that.

Travis build link for reference: https://travis-ci.org/prometheus/alertmanager/builds/489679923

beorn7 · 2019-02-06T19:43:50Z

I think a bunch of things in our build are pulled in with current versions. That might explain changing behavior (and is at odds with the go-modules approach of reproducible builds).

beorn7 · 2019-02-06T23:08:15Z

I'll get to the bottom of this tomorrow with a clear head… Very mysterious. 🤔

simonpasquier · 2019-02-07T16:21:21Z

Regarding the errcheck failures, I suspect that Travis CI messes up with GOPATH. I'm trying https://github.com/prometheus/alertmanager/pull/1748to investigate/fix it.

beorn7 · 2019-02-07T17:48:05Z

Rebased on top of master with @simonpasquier 's and my fixes. Let's see what's happening now.

beorn7 · 2019-02-07T18:15:11Z

Tadaaa! 🎉
It worked.
@mxinden now we can get to the normal review. Waiting for your further comments.

mxinden · 2019-02-08T11:14:55Z

api/api.go

+)
+
+func init() {
+	prometheus.MustRegister(requestsInFlight)


I am not a big fan of the global metric registry. It might be less verbose but also implicit and more difficult when it comes to vendoring. I am trying the steer this project away from it. What do you think in this regard? Would passing a Registerer to API.New() also be an option?

I'm not a fan of the global registry, either. I just wanted to stay consistent with the way it is and then have a separate refactoring to make it local.

But now I have seen what you did in #1741, which is already merged. Thus, a incremental migration to local Registerers is already ongoing. Happy to follow that path here.

api/api.go

api/v1/api.go

simonpasquier · 2019-02-08T13:48:09Z

Nothing much to add to @mxinden's comments from my side.

beorn7 · 2019-02-08T18:35:45Z

Responded to the comments, but haven't yet pushed the commit to actually address them.
Give me some time...

cmd/alertmanager/main.go

The default concurrency limit is max(GOMAXPROCS, 8). That should not imply that each GET requests eats a whole CPU. It's more to get some reasonable heuristics for the processing power of the hosting machine (while allowing at least 8 concurrent requests even on the smallest machines). As GET requests can easily overload the Alertmanager, rendering it incapable of doing its main task, namely sending alert notifications, we need to limit GET requests by default. In contrast, no timeout is set by default. The http.TimeoutHandler inovkes quite a bit of machinery behind the scenes, in particular an additional layer of buffering. Thus, we should first get a bit of experience with it before we consider enforcing a timeout by default, even if setting a timeout is in general the safer setting for resiliency. Signed-off-by: beorn7 <beorn@soundcloud.com>

The context is created by the http.TimeoutHandler we use to set the timeout. I believe this is the only endpoint where propagating the timeout is feasible and needed. Signed-off-by: beorn7 <beorn@soundcloud.com>

While the newly added in-flight instrumentation works for all GET requests, the existing HTTP instrumentation omits api/v2 calls. This commit adds a TODO note about that. Signed-off-by: beorn7 <beorn@soundcloud.com>

beorn7 · 2019-02-12T17:01:45Z

Sorry for the delay. Got sidetracked all the time. Finally pushed that new commit. @stuartnelson3 is gone for vacation again. @mxinden please have another look.

Most importantly, `api.New` now takes an `Options` struct as an argument, which allows some other things done here as well: - Timout and concurrency limit are now in the options, streamlining the registration and the implementation of the limiting middleware. - A local registry is used for metrics, and the metrics used so far inside any of the api packages are using it now. The 'in flight' metric now contains the 'get' as a method label. I have also added a TODO to instrument other methods in the same way (otherwise, the label doesn't reall make sense, semantically). I have also added an explicit error counter for requests rejected because of the concurrency limit. (They also show up as 503s in the generic HTTP instrumentation (or they would, if v2 were instrumented, too), but those 503s might have a number of reasons, while users might want to alert on concurrency limit problems explicitly). Signed-off-by: beorn7 <beorn@soundcloud.com>

beorn7 · 2019-02-13T17:10:03Z

@mxinden Do you think you could have a look at this any time soon?

mxinden

Just one small thing. Otherwise this looks good to me.

mxinden · 2019-02-13T17:58:45Z

api/api.go

+	requestsInFlight         prometheus.Gauge
+	concurrencyLimitExceeded prometheus.Counter
+	timeout                  time.Duration
+	inFlightSem              chan struct{}


This is only used in the handler, right? How about initializing it in the handler constructor closure?

The problem is that we call limitHandler twice, once to handle the router handler, once to handle the apiv2 handler. If we moved the inFlightSem into limitHandler, we would limit the GET requests separately for each one. But we want one limit for all of the HTTP handling. (That was the reason why I had a function that returns a function (the middleware) that returns a function (the HandlerFunc) in the previous version. I could avoid that confusion by having inFlightSem as a member of API and limitHandler a method of API.

@mxinden did my comment clarify the issue? Or do you think this should be changed in another way?

mxinden · 2019-02-13T18:00:57Z

api/api.go

+	inFlightSem              chan struct{}
+}
+
+// Options for the creation of an API object. Alerts, Silences, and StatusFunc


Thanks for adding detailed comments here.

beorn7 · 2019-02-15T15:17:10Z

Thanks @simonpasquier . @mxinden I assume things are clarified now. We can still rework this if needed. I just would to like to get this merged as #1733 needs to address some merge conflicts with it and I have yet another change on top of all that in my pipeline. Thus, I'm merging this now to get all those ends tied up. It also unblocks your work on #1744.

beorn7 requested a review from mxinden February 6, 2019 17:43

beorn7 mentioned this pull request Feb 6, 2019

*: Introduce config coordinator bundling config specific logic #1744

Merged

mxinden reviewed Feb 6, 2019

View reviewed changes

api/api.go Outdated Show resolved Hide resolved

api/api.go Outdated Show resolved Hide resolved

api/api.go Outdated Show resolved Hide resolved

beorn7 force-pushed the beorn7/api branch 2 times, most recently from 0318737 to 5374774 Compare February 7, 2019 15:21

beorn7 force-pushed the beorn7/api branch from 5374774 to 467d1ff Compare February 7, 2019 17:46

mxinden reviewed Feb 8, 2019

View reviewed changes

simonpasquier self-requested a review February 8, 2019 13:47

stuartnelson3 reviewed Feb 11, 2019

View reviewed changes

cmd/alertmanager/main.go Outdated Show resolved Hide resolved

beorn7 added 3 commits February 11, 2019 19:34

Propagate timeout to alert listing via context

4747fd9

The context is created by the http.TimeoutHandler we use to set the timeout. I believe this is the only endpoint where propagating the timeout is feasible and needed. Signed-off-by: beorn7 <beorn@soundcloud.com>

Add HTTP instrumentation for GET requests in flight

3382a0e

While the newly added in-flight instrumentation works for all GET requests, the existing HTTP instrumentation omits api/v2 calls. This commit adds a TODO note about that. Signed-off-by: beorn7 <beorn@soundcloud.com>

beorn7 force-pushed the beorn7/api branch from 467d1ff to 7f79c2a Compare February 12, 2019 17:00

beorn7 force-pushed the beorn7/api branch from 7f79c2a to 6788086 Compare February 12, 2019 17:05

beorn7 force-pushed the beorn7/api branch from 6788086 to 21de9ff Compare February 12, 2019 17:42

mxinden reviewed Feb 13, 2019

View reviewed changes

simonpasquier approved these changes Feb 15, 2019

View reviewed changes

beorn7 merged commit da6e2a8 into master Feb 15, 2019

beorn7 deleted the beorn7/api branch February 15, 2019 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce concurrency limit for GET requests and a general timeout for HTTP #1743

Introduce concurrency limit for GET requests and a general timeout for HTTP #1743

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

mxinden commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

simonpasquier commented Feb 7, 2019

beorn7 commented Feb 7, 2019

beorn7 commented Feb 7, 2019

mxinden Feb 8, 2019

beorn7 Feb 8, 2019

simonpasquier commented Feb 8, 2019

beorn7 commented Feb 8, 2019

beorn7 commented Feb 12, 2019

beorn7 commented Feb 13, 2019

mxinden left a comment

mxinden Feb 13, 2019

beorn7 Feb 13, 2019

beorn7 Feb 15, 2019

mxinden Feb 13, 2019

beorn7 commented Feb 15, 2019

Introduce concurrency limit for GET requests and a general timeout for HTTP #1743

Introduce concurrency limit for GET requests and a general timeout for HTTP #1743

Conversation

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

mxinden commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

simonpasquier commented Feb 7, 2019

beorn7 commented Feb 7, 2019

beorn7 commented Feb 7, 2019

mxinden Feb 8, 2019

Choose a reason for hiding this comment

beorn7 Feb 8, 2019

Choose a reason for hiding this comment

simonpasquier commented Feb 8, 2019

beorn7 commented Feb 8, 2019

beorn7 commented Feb 12, 2019

beorn7 commented Feb 13, 2019

mxinden left a comment

Choose a reason for hiding this comment

mxinden Feb 13, 2019

Choose a reason for hiding this comment

beorn7 Feb 13, 2019

Choose a reason for hiding this comment

beorn7 Feb 15, 2019

Choose a reason for hiding this comment

mxinden Feb 13, 2019

Choose a reason for hiding this comment

beorn7 commented Feb 15, 2019