ingester: don't log errors that cause OOMs, using interface #5581

bboreham · 2023-07-25T15:00:34Z

What this PR does

Not-ready and over-max-inflight can be emitted when the server is overloaded, in which case we don't want to spend more resource logging the fact.

The max-inflight errors will be recorded in metrics added in #5551.

Temporarily using a branch from upstream weaveworks/common, to check Mimir still passes CI.
Update on #5494

Checklist

NA Tests updated
NA Documentation added
NA CHANGELOG.md updated

pkg/distributor/distributor.go

56quarters

LGTM. I like this approach compared to other one (adding DoNotLogError in common).

vendor/github.com/weaveworks/common/middleware/grpc_logging.go

56quarters · 2023-07-25T19:57:10Z

vendor/github.com/weaveworks/common/middleware/grpc_logging.go

@@ -17,6 +17,11 @@ const (
 	errorKey = "err"
 )

+// If an error implements Observe(), it will get called and GRPCServerLog will do nothing.
+type Observer interface {


Can we document (or codify) some of the expectations of implementers of this? For example, we expect if you implement Observer you also implement error and have a .Unwrap() method.

we expect if you implement Observer you also implement error and have a .Unwrap() method

I don't think we expect that, right? You'll have to implement error because this is checked on an error, but we don't need that. And there's no need for the Unwrap() method.

pracucci

Also looking at #5584, I’m not a big fan of Observe(). We’re going to have the logging logic scattered around. I’m wondering if we could simply achieve the same with a function which tells whether the error should be logged or not (return bool), instead of delegating the log to the error's Observe().

bboreham · 2023-07-26T07:38:49Z

Also looking at #5584, I’m not a big fan of Observe().

How about ShouldLog() ? weaveworks/common@9927ce6

bboreham · 2023-07-26T07:49:10Z

pkg/distributor/distributor.go

@@ -45,6 +44,7 @@ import (
 	"github.com/grafana/mimir/pkg/mimirpb"
 	"github.com/grafana/mimir/pkg/util"
 	"github.com/grafana/mimir/pkg/util/globalerror"
+	log_util "github.com/grafana/mimir/pkg/util/log"


util_log for consistency.

pracucci · 2023-07-26T07:51:36Z

Also looking at #5584, I’m not a big fan of Observe().

How about ShouldLog() ? weaveworks/common@9927ce6

Looks better to me. Do we need to pass duration time.Duration? What will be used for? I can't see it used in #5584.

bboreham · 2023-07-26T12:48:56Z

Do we need to pass duration time.Duration? What will be used for?

I took the five parameters from the previous version, and tried to imagine which ones could be used.
"I want to log all operations over 5 seconds" seemed possible, so I left the duration in.
Getting something from the Context, perhaps a tracing span, also seemed likely.

I know it's YAGNI, but we can never change that interface (that would silently break all users).
We can add more interfaces.

This allows us to decorate them with extra information for gRPC responses and our logging middleware (to prevent them from being logged, increasing resource usage). Related #5581 Related weaveworks/common#299 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

pracucci · 2023-07-28T05:11:06Z

vendor/github.com/weaveworks/common/middleware/logging.go

@@ -91,9 +83,6 @@ func (l Log) Wrap(next http.Handler) http.Handler {
 		statusCode, writeErr := wrapped.getStatusCode(), wrapped.getWriteError()

 		if writeErr != nil {
-			if errors.Is(writeErr, DoNotLogError{}) {


Isn't required anymore? Can you explain why was it removed from here?

This error is the return from Write(); I don't see how it can be one of our wrapped errors.

Also I looked at the high-volume log lines that have been problemmatic and they were all from grpc_logging, none from http request logging.

pracucci · 2023-07-28T05:36:20Z

I pushed a tiny change to a test, to improve it. Hope you don't mind: 0108bb6

pracucci

LGTM (modulo a question)

colega · 2023-07-28T07:07:45Z

ShouldLog also works for me, although I don't like the fact that the error won't say the sampling. When you're reading the logs with no context, you might easily miss the fact that the error is sampled if the error doesn't say that explicitly.

pracucci · 2023-07-28T08:10:25Z

ShouldLog also works for me, although I don't like the fact that the error won't say the sampling. When you're reading the logs with no context, you might easily miss the fact that the error is sampled if the error doesn't say that explicitly.

I agree (also mentioned in the related design doc). I was waiting for the next PR that will introduce the sampling, to stress about this ;)

bboreham · 2023-07-29T06:43:42Z

Upstream PR in weaveworks/common has merged, so I rebased. Please check I didn't drop any of your commits.

the next PR that will introduce the sampling

PR #5584 is sampling 6 or so errors.

pracucci · 2023-07-31T07:42:08Z

go.mod

-// Use a fork of weaveworks/common while we work out if there is a better design for https://github.com/weaveworks/common/pull/293
-replace github.com/weaveworks/common => github.com/weaveworks/common v0.0.0-20230714173453-d1f8877b91ce


Note to reviewers: removed because weaveworks/common#299 has been merged.

Using weaveworks/common#299 which is a more flexible approach. Note the check disappeared from `logging.go`, because it was a mistake to check that error. It comes from `io.Writer`, it won't be an app- level error.

Signed-off-by: Marco Pracucci <marco@pracucci.com>

This allows us to decorate them with extra information for gRPC responses and our logging middleware (to prevent them from being logged, increasing resource usage). Related #5581 Related weaveworks/common#299 Signed-off-by: Nick Pillitteri <nick.pillitteri@grafana.com>

…gging for them (#5585) This allows us to decorate them with extra information for gRPC responses and our logging middleware (to prevent them from being logged which is expensive). Related #5581 Related weaveworks/common#299

bboreham requested a review from a team as a code owner July 25, 2023 15:00

pracucci reviewed Jul 25, 2023

View reviewed changes

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

bboreham force-pushed the no-log-error2 branch from f42054c to 7f13730 Compare July 25, 2023 16:30

56quarters mentioned this pull request Jul 25, 2023

Create base error type for ingester per-instance errors and remove logging for them #5585

Merged

3 tasks

56quarters approved these changes Jul 25, 2023

View reviewed changes

vendor/github.com/weaveworks/common/middleware/grpc_logging.go Outdated Show resolved Hide resolved

56quarters reviewed Jul 25, 2023

View reviewed changes

pracucci reviewed Jul 26, 2023

View reviewed changes

bboreham commented Jul 26, 2023

View reviewed changes

pracucci reviewed Jul 28, 2023

View reviewed changes

pracucci approved these changes Jul 28, 2023

View reviewed changes

bboreham force-pushed the no-log-error2 branch from 0108bb6 to 1b8913d Compare July 28, 2023 13:44

pracucci reviewed Jul 31, 2023

View reviewed changes

bboreham and others added 2 commits July 31, 2023 08:52

Don't log errors that cause OOMs, using interface

e7bd0fa

Using weaveworks/common#299 which is a more flexible approach. Note the check disappeared from `logging.go`, because it was a mistake to check that error. It comes from `io.Writer`, it won't be an app- level error.

Improved TestIngester_inflightPushRequests

c4ba830

Signed-off-by: Marco Pracucci <marco@pracucci.com>

bboreham force-pushed the no-log-error2 branch from a3c1166 to c4ba830 Compare July 31, 2023 07:53

bboreham merged commit 1e5298e into main Jul 31, 2023
28 checks passed

bboreham deleted the no-log-error2 branch July 31, 2023 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ingester: don't log errors that cause OOMs, using interface #5581

ingester: don't log errors that cause OOMs, using interface #5581

bboreham commented Jul 25, 2023 •

edited

Loading

56quarters left a comment

56quarters Jul 25, 2023 •

edited

Loading

colega Jul 28, 2023

pracucci left a comment

bboreham commented Jul 26, 2023

bboreham Jul 26, 2023

pracucci commented Jul 26, 2023

bboreham commented Jul 26, 2023

pracucci Jul 28, 2023

bboreham Jul 28, 2023

bboreham Jul 28, 2023

pracucci commented Jul 28, 2023

pracucci left a comment

colega commented Jul 28, 2023

pracucci commented Jul 28, 2023

bboreham commented Jul 29, 2023

pracucci Jul 31, 2023

		// Use a fork of weaveworks/common while we work out if there is a better design for https://github.com/weaveworks/common/pull/293
		replace github.com/weaveworks/common => github.com/weaveworks/common v0.0.0-20230714173453-d1f8877b91ce

ingester: don't log errors that cause OOMs, using interface #5581

ingester: don't log errors that cause OOMs, using interface #5581

Conversation

bboreham commented Jul 25, 2023 • edited Loading

What this PR does

Checklist

56quarters left a comment

Choose a reason for hiding this comment

56quarters Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

colega Jul 28, 2023

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

bboreham commented Jul 26, 2023

bboreham Jul 26, 2023

Choose a reason for hiding this comment

pracucci commented Jul 26, 2023

bboreham commented Jul 26, 2023

pracucci Jul 28, 2023

Choose a reason for hiding this comment

bboreham Jul 28, 2023

Choose a reason for hiding this comment

bboreham Jul 28, 2023

Choose a reason for hiding this comment

pracucci commented Jul 28, 2023

pracucci left a comment

Choose a reason for hiding this comment

colega commented Jul 28, 2023

pracucci commented Jul 28, 2023

bboreham commented Jul 29, 2023

pracucci Jul 31, 2023

Choose a reason for hiding this comment

bboreham commented Jul 25, 2023 •

edited

Loading

56quarters Jul 25, 2023 •

edited

Loading