[JSON-API] Fix logging of internal server errors #12822
Conversation
realvictorprm
requested review from
S11001001 and
stefanobaghino-da
as code owners
|
@realvictorprm Can you create a ticket for this, please? It would be quite important to make sure we can keep track of the version when this bug was first introduced so that we can answer to support requests about it and possibly think about backports if and when relevant. Please refer to that in the changelog as well (saying that "server errors are now properly logged again" without saying since when this was an issue could be a problem for users). Thanks. |
|
@S11001001 can you review this, please? |
| )(implicit metrics: Metrics, jsonWriter: JsonWriter[A]): Route = | ||
| )(implicit | ||
| lc: LoggingContextOf[InstanceUUID with RequestID], | ||
| metrics: Metrics, | ||
| jsonWriter: JsonWriter[A], | ||
| ): Route = | ||
| responseToRoute(httpResponse(res)) | ||
| private def toRoute(res: => Future[Error \/ SearchResult[Error \/ JsValue]]): Route = | ||
| private def toRoute(res: => Future[Error \/ SearchResult[Error \/ JsValue]])(implicit | ||
| lc: LoggingContextOf[InstanceUUID with RequestID] | ||
| ): Route = |
There was a problem hiding this comment.
This will definitely conflict with main.
changelog_begin - [HTTP-JSON] Internal server errors are now properly logged again changelog_end
realvictorprm
force-pushed
the
http-json/fix-internal-server-error-logging
branch
from
9fffc33 to
f36f7ef
Compare
I don't know yet when this started to happened and need to investigate myself first. However I do remember my past idea that it is probably linked to when we refactored our code to not expose internal server errors anymore via the response. |
I understand, please do the research. Checking only major versions is fine, so bisecting manually should take a short time. |
|
OK I think the "bug" was introduced with #11184 |
|
I think this solution probably isn't perfect because if I understand the code correctly, errors from futures and sources will be logged twice with these changes. |
|
OK I think best is if we merge this first and make a follow up PR where I fix the duplicate logging (again, did smth like that already in the past 😂 ). |
realvictorprm
deleted the
http-json/fix-internal-server-error-logging
branch
Fixes #12887
Changes we did in the past caused that internal server errors were not logged anymore. With these changes they will be logged again which is quite helpful for understanding what is going on if smth goes wrong 😂
changelog_begin
changelog_end
Pull Request Checklist
CHANGELOG_BEGINandCHANGELOG_ENDtagsNOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with
/AzurePipelines runtotrigger the build.