-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change log level of retry simple attempt #86
Change log level of retry simple attempt #86
Conversation
6640edb
to
3f5be36
Compare
@@ -51,9 +51,8 @@ public async Task Invoke(IMessageContext context, MiddlewareDelegate next) | |||
} | |||
} | |||
|
|||
this.logHandler.Error( | |||
this.logHandler.Verbose( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you that the log level of a retry attempt in this context should be Verbose, nevertheless, when the message fails the first time, I think the log level should be Error so the users will know that something wrong is happening. What do you think about this? Here is a suggestion:
var logMsg = $"Exception captured by {nameof(RetrySimpleMiddleware)}. Retry in process.";
var logObj = new
{
AttemptNumber = attemptNumber,
WaitMilliseconds = waitTime.TotalMilliseconds,
PartitionNumber = context.ConsumerContext.Partition,
Worker = context.ConsumerContext.WorkerId,
//Headers = context.HeadersAsJson(),
//Message = context.Message.ToJson(),
ExceptionType = exception.GetType().FullName,
ExceptionMessage = exception.Message
};
if (attemptNumber == 1)
{
this.logHandler.Error(logMsg, exception, logObj);
}
else
{
this.logHandler.Verbose(logMsg, logObj);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi ! We are also looking for this feature, but could we log the last attempt instead of the first one? If a message is retried only once and then passes, it means the issue is temporary and we don't have to deal with it, but if after all the retries we still can't consume, then we have a problem that we have to deal.
After some conversation with the finance open-source team, we concluded that this PR shouldn't be merged. The users may accomplish this issue using the retry durable and using some metadata that flows in the headers of the messages. In the property |
Description
The log level of attempts of the simple retry definition is Error. Since this is a retry attempt, the log level should be Verbose.
This PR changes the log level of the retry attempt to Verbose.
Fixes #85
How Has This Been Tested?
Test the consumption of events locally, and ensure that the log level of the retry attempt is verbose.
Checklist
Disclaimer
By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement