Closed dxuhuang closed 7 years ago
Merging #589 into develop will decrease coverage by
0.06%
. The diff coverage is84%
.
@@ Coverage Diff @@
## develop #589 +/- ##
===========================================
- Coverage 84.94% 84.87% -0.07%
===========================================
Files 99 99
Lines 4431 4451 +20
Branches 378 379 +1
===========================================
+ Hits 3764 3778 +14
- Misses 667 673 +6
Impacted Files | Coverage Δ | |
---|---|---|
...c/main/scala/colossus/service/RequestHandler.scala | 88.88% <100%> (ø) |
:arrow_up: |
...rc/main/scala/colossus/service/ServiceServer.scala | 90.83% <83.33%> (-2.17%) |
:arrow_down: |
...s/src/main/scala/colossus/metrics/Collection.scala | 97.43% <0%> (-2.57%) |
:arrow_down: |
.../scala/colossus/metrics/collectors/Histogram.scala | 90.57% <0%> (-0.73%) |
:arrow_down: |
...sus/src/main/scala/colossus/service/Callback.scala | 88.37% <0%> (ø) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update b3853de...6c118cf. Read the comment docs.
Just trying to think how it would be overriden:
def formatError(error: Throwable): String = {
error match {
case DroppedReplyException => "DroppedReplyException"
case _ => // get stack trace
}
}
The problem is that it is a bit of work to extract the stack trace since that functionality is contained within the logger. Maybe it would be more useful to be able to indicate whether an error should be logged. There could be three levels:
It would be great if this stuff can be configurable (like the metrics) so by default the DroppedReplyException
would only log name, but everything else would be name and stack trace.
I like the general idea. What do you guys think about having formatError
also take in the failed request as a parameter, and then using the returned string as the entire error message? That way the formatter has complete control over the error message.
I also agree with what @benblack86 said. Having an ADT like:
sealed trait LogAction
case object DoNotLog extends LogAction
case class Log(message: String, includeStackTrace: Boolean) extends LogAction
or something similar seems like a good idea. Also #414 does mention doing something for exceptions like DroppedReplyException
. I'm ok if that's not addressed in this PR, but we should then create another issue for it.
bump. will add stuff in conf soon
Looks good so far.
bump @DanSimon
Might be wrong, but I don't think the RequestFormatter
is documented. Can you add/update the docs.
bump
bump
@benblack86 @DanSimon @aliyakamercan @jlbelmonte @amotamed
My stab at https://github.com/tumblr/colossus/issues/414