fix: don't log at error level in lease timeout #2151
Conversation
stuartwdouglas
requested review from
a team and
matt2e
and removed request for
a team
stuartwdouglas
force-pushed
the
stuartwdouglas/error-filtering
branch
2 times, most recently
from
a0dfc60 to
cfd52fc
Compare
backend/controller/leader/leader.go
Outdated
| return false | ||
| } | ||
| return true | ||
| } else { |
There was a problem hiding this comment.
Looks like it'd be neater to do if !ok and then return early
common/configuration/asm_follower.go
Outdated
| @@ -51,7 +53,14 @@ func (f *asmFollower) sync(ctx context.Context, values *xsync.MapOf[Ref, SyncedV | |||
| IncludeValues: &includeValues, | |||
| })) | |||
| if err != nil { | |||
| return fmt.Errorf("error getting secrets list from leader: %w", err) | |||
| if f.errorFilter.ReportLeaseError() { | |||
There was a problem hiding this comment.
I do think this is too general, like was mentioned on this morning's call. What @alecthomas said about responses that include errors vs network errors feels like a good way to differentiate.
There was a problem hiding this comment.
Do we have an example somewhere of how to determine the type of error? The library docs mention connect.Error but that does not seem to match what is in the datadog logs.
I never thought I would miss Java checked exceptions, but they do make situations like this easier.
There was a problem hiding this comment.
Maybe not useful for this case, but here's an example of checking connect errors: https://github.com/TBD54566975/ftl/blob/main/backend/controller/ingress/handler.go#L55-L62
There was a problem hiding this comment.
Thanks, I have updated it to use the return codes to decide on if it should attempt to filter
There was a problem hiding this comment.
Looks like a good direction @stuartwdouglas as this does feel like something that would be useful for more things that end up using leader/follower.
I think you mentioned on the call this morning that you thought it might be useful for anything that used leases? I'm less sure about that but maybe I havent thought of use cases, or maybe I'm misremembering. Either way my feel is what you've done in the code here is the right place, but I'm interested to hear if you had other ideas?
stuartwdouglas
force-pushed
the
stuartwdouglas/error-filtering
branch
6 times, most recently
from
726d3ef to
8dd27a1
Compare
We don't want error alerts if a controller is just failing over, this PR adds some infrasturcture to allow for a lower level of error reporting when the errors all occur within the lease TTL. fixes #2133
stuartwdouglas
force-pushed
the
stuartwdouglas/error-filtering
branch
from
8dd27a1 to
dc6903d
Compare
|
@matt2e are you happy with this now? |
We don't want error alerts if a controller is just failing over, this PR adds some infrasturcture to allow for a lower level of error reporting when the errors all occur within the lease TTL.
fixes #2133