-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Add more info to UnrecognizedMediaType #43100
Conversation
@@ -29,6 +29,9 @@ internal static partial class HttpLoggingExtensions | |||
[LoggerMessage(5, LogLevel.Debug, "Decode failure while converting body.", EventName = "DecodeFailure")] | |||
public static partial void DecodeFailure(this ILogger logger, Exception ex); | |||
|
|||
[LoggerMessage(6, LogLevel.Debug, "Unrecognized Content-Type for body.", EventName = "UnrecognizedMediaType")] | |||
public static partial void UnrecognizedMediaType(this ILogger logger); | |||
[LoggerMessage(6, LogLevel.Debug, "Unrecognized Content-Type for request body.", EventName = "RequestUnrecognizedMediaType")] |
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.
Let's keep the same id for both logs to avoid any possible breakage no matter how unlikely. We should be able to parameterize just the request/response part of the message.
I was going to suggest including what the unrecognized content-type was exactly. It seems useful for debugging, but then I realize most people logging the body are probably logging the header separately anyway.
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 was going to suggest including what the unrecognized content-type was exactly.
Should there be a different log when there isn't a media type header?
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.
Is that a common scenario?
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.
Probably? Isn't that what the original issue was sort of about?
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.
Agreed. An "Unrecognized Content-Type" log when there isn't a "Content-Type" to begin with is super confusing. I would not log anything at all in that case.
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.
Added a new log message & a test
Is there an issue or other feedback about this? |
|
This only ever gets fired for the request body, not the response body.