Add tag "loggerName" to LogbackMetrics #2475
Conversation
|
@michalkowol Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
|
@michalkowol Thank you for signing the Contributor License Agreement! |
There was a problem hiding this comment.
Thanks for the pull request. This is interesting and I can see how it would be useful. I do worry if it is a good default, however, given the potential for high cardinality. Perhaps it would be better to make this possible to configure without it being the default. We would also want to do something similar for our Log4j2Metrics.
| // cannot use logger.isEnabledFor(level), as it would cause a StackOverflowError by calling this filter again! | ||
| if (level.isGreaterOrEqual(logger.getEffectiveLevel())) { | ||
| switch (level.toInt()) { | ||
| case Level.ERROR_INT: | ||
| errorCounter.increment(); | ||
| getOrCreateErrorCounter(loggerName).increment(); |
There was a problem hiding this comment.
"get or create" is already the behavior when registering a meter, so I don't think the Maps add value here unless I'm missing something.
Motivation
I find
logback.eventsvery useful to determine the overall condition of the system.logback.eventswithlevel=erroris one of the most important metrics in few projects I am working on.The missing was information which logger/class triggered that logback event.
To provide even greater insight I think this metric should also include
loggerName.I am testing in on prod (about 15K RPS) and it works very well. My whole team really appreciate this new ability to get a better understanding what happens in our system