Add Throwable "cause" variants to warning log API #30389
Conversation
|
Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK ΤO ΤESΤ' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged. For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask! |
|
@emanb29, the build is failing because of bin-incompatibility with previous API. You will need to add a mima exclusion for ProblemFilters.exclude[ReversedMissingMethodProblem("akka.event.LoggingAdapter.notifyWarning")You do that by adding a file similar to Note that you will need add the file under Now, concerning the addition, I checked how logback and log4j do the same and indeed they do have overloaded variants accepting a I'm not sure what was the rationale behind the decision to not do the same in akka logging. Maybe something worth reconsider. Maybe others (@johanandren, @jrudolph, @patriknw) have more insight on why it wasn't added. |
Co-authored-by: Renato Cavalcanti <renato@cavalcanti.be>
|
Thanks @octonato! I made a copy/paste error but I found the travis command and am re-checking locally before updating the branch again. |
This might be an actual incompatibility, as this is a public class that might be implemented by third parties. If we want to include this, we might want to include a default implementation that calls the existing
I don't know but maybe it was just missed in the beginning and compatibility requirements prevented it afterwards? Personally, I'm not against adding it when the compat issues can be resolved. |
That's true, but I'm wondering which scenario you have in mind where a method addition could cause a problem. If I'm using such 3rd party library and I update my Akka version, the Akka version used by the 3rd party library will be evicted, no? |
|
The third party library could contain a subclass, that is not recompiled by upgrading Akka so does not have the new method. The third party library-case is in general the most problematic scenario with binary compatibility, compared to a users own leaf-project where sources are easier to recompile to fix a binary compatibility. Not sure how likely that is in this case but we should step carefully. A default impl might work, but we should verify that. |
I created a small project containing three sub-projects: Adding a default impl for the new method as suggested by @jrudolph does the trick (as expected). Please, check readme file to understand how I tested it. I think this test covers the case here. Let me know if I missed something. |
|
Double checked the idea of default implementation with both 2.13 and 2.12 and that works fine (because Scalac turns the trait into an interface with |
Co-authored-by: Johan Andrén <johan@markatta.com>
| @@ -1199,6 +1199,7 @@ trait LoggingAdapter { | |||
| protected def notifyError(message: String): Unit | |||
| protected def notifyError(cause: Throwable, message: String): Unit | |||
| protected def notifyWarning(message: String): Unit | |||
| protected def notifyWarning(cause: Throwable, message: String): Unit = notifyWarning(message) | |||
There was a problem hiding this comment.
| protected def notifyWarning(cause: Throwable, message: String): Unit = notifyWarning(message) | |
| protected def notifyWarning(@unused cause: Throwable, message: String): Unit = notifyWarning(message) |
| @@ -1981,6 +2025,8 @@ class BusLogging(val bus: LoggingBus, val logSource: String, val logClass: Class | |||
| bus.publish(Error(cause, logSource, logClass, message, mdc)) | |||
| protected def notifyWarning(message: String): Unit = | |||
| bus.publish(Warning(logSource, logClass, message, mdc)) | |||
| protected def notifyWarning(cause: Throwable, message: String): Unit = | |||
There was a problem hiding this comment.
| protected def notifyWarning(cause: Throwable, message: String): Unit = | |
| override protected def notifyWarning(cause: Throwable, message: String): Unit = |
| @@ -0,0 +1,2 @@ | |||
| # additional overload added | |||
| ProblemFilters.exclude[ReversedMissingMethodProblem]("akka.event.LoggingAdapter.notifyWarning") | |||
There was a problem hiding this comment.
I tried it out and it looks like we can delete this exclude now and MiMa is still happy
| @@ -1981,6 +2025,8 @@ class BusLogging(val bus: LoggingBus, val logSource: String, val logClass: Class | |||
| bus.publish(Error(cause, logSource, logClass, message, mdc)) | |||
| protected def notifyWarning(message: String): Unit = | |||
| bus.publish(Warning(logSource, logClass, message, mdc)) | |||
| protected def notifyWarning(cause: Throwable, message: String): Unit = | |||
| bus.publish(Warning(cause, logSource, logClass, message, mdc)) | |||
There was a problem hiding this comment.
Funny that we had prepared for it with that Warning4 class, but not exposing it in the LoggingAdapter. Must have been an oversight.
|
Completed in #31525 |
|
Merged to main as 044bb2d |
Adds variants to
log.warningto bring it closer tolog.error, in particular, adding acause: Throwable.A (not-so-hypothetical) use-case for this is an application for which there are a set of recoverable errors, which do not interfere with application functionality, but do indicate an error on the end-user's part. In such a case, it can be helpful to log a warning, and convenient to pass along the Throwable that caused said warning, in a way that is consistent with
log.error