Make distributor log ingester details when a push request fails #6801
Conversation
…logs Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
duricanikolic
force-pushed
the
yuri/better-err-msgs
branch
from
31f04dc
to
7d7e7ff
Compare
duricanikolic
changed the title
duricanikolic
force-pushed
the
yuri/better-err-msgs
branch
from
7d7e7ff
to
fdf9361
Compare
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
duricanikolic
force-pushed
the
yuri/better-err-msgs
branch
from
fdf9361
to
b7743cb
Compare
pkg/distributor/errors_test.go
Outdated
| @@ -20,6 +20,10 @@ import ( | |||
| "github.com/grafana/mimir/pkg/mimirpb" | |||
| ) | |||
|
|
|||
| var ( | |||
| ingesterID = "ingester-25" | |||
There was a problem hiding this comment.
[nit] As a general good practice, I suggest to not define such generic global variables (BTW it could be a const) because it may be easy to misuse in tests. Instead, you could just define this const in each test where it's used (they're few).
There was a problem hiding this comment.
I have implemented this fix. Thank you
| @@ -65,7 +65,7 @@ import ( | |||
| ) | |||
|
|
|||
| var ( | |||
| errFail = httpgrpc.Errorf(http.StatusInternalServerError, "Fail") | |||
| errFail = status.Error(codes.Internal, "Fail") | |||
There was a problem hiding this comment.
errFail is used in mockIngester struct as an example of error returned by that ingester client. After the recent error handling refactoring, it is expected that ingesters will not return errors with HTTP status codes anymore. This is still the default behaviour, but will soon be deprecated.
As a consequence, I replaced the current errFail value with an error with a pure gRPC code.
It is worth noting that this change could be done in a separate PR.
| @@ -1139,8 +1139,8 @@ func TestDistributor_PushQuery(t *testing.T) { | |||
| assert.EqualError(t, err, tc.expectedError.Error()) | |||
|
|
|||
| // Assert that downstream gRPC statuses are passed back upstream | |||
| _, ok := httpgrpc.HTTPResponseFromError(err) | |||
There was a problem hiding this comment.
This change is related to the change discussed above.
The previous version of this test was ensuring that the error returned by distributor.QueryStream() was produced by using the httpgrpc package. This is, anyway, not true. Before error handling improvement, it was the case sometimes, but after error handling improvement it is not the case anymore. The reason why the test was not failing was because we were using errFail defined as httpgrpc.Errorf(http.StatusInternalServerError, "Fail") as the only error returned by mockIngester, and that made the test successful.
Again, this could be done in a separate PR.
There was a problem hiding this comment.
I'm fine with the change in this PR, but as a general good practice I recommend to keep each PR focused on a single scope (ideally what the PR title says). As a retrospective, this test fix may have been a dedicated PR, which would have been easier to understand and in the future easier to track when we look back at code changes via git (each PR commits are squashed into a single commit).
Just a feedback for the next time.
Signed-off-by: Yuri Nikolic <durica.nikolic@grafana.com>
What this PR does
Currently, if an ingester fails to push a request, the distributor that triggered the push loggs an error message like this:
This message doesn't contain any detail about which ingester was involved. This PR enriches the logged error with ingester's ID, so the improved log message will have the following format:
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.