Add an InstrumentedAppender constructor that allows passing a CollectorRegistry #690
Conversation
…torRegistry Signed-off-by: Matthew Dolan <MatthewDolan@users.noreply.github.com>
MatthewDolan
force-pushed
the
dolan-2021-09-07-instrumented-appender
branch
from
fa49d45 to
83238c6
Compare
MatthewDolan
changed the title
| public static class DefaultTest extends Base { | ||
| @Before | ||
| public void setUp() throws Exception { | ||
| this.registry = CollectorRegistry.defaultRegistry; | ||
|
|
||
| appender.doAppend(event); | ||
| this.appender = new InstrumentedAppender(); | ||
|
|
||
| assertEquals(1, getLogLevelCount("error")); | ||
| super.setUp(); | ||
| } | ||
| } | ||
|
|
||
| private int getLogLevelCount(String level) { | ||
| return CollectorRegistry.defaultRegistry.getSampleValue(COUNTER_NAME, | ||
| new String[]{"level"}, new String[]{level}).intValue(); | ||
| public static class InstanceTest extends Base { | ||
| @Before | ||
| public void setUp() throws Exception { | ||
| this.registry = new CollectorRegistry(); | ||
|
|
||
| this.appender = new InstrumentedAppender(this.registry); | ||
|
|
||
| super.setUp(); | ||
| } |
There was a problem hiding this comment.
Thanks for the PR, the changes to the production code are looking good.
However, in the test code I think that the inner classes DefaultTest and InstanceTest are not executed in the Maven build. If I change an assertEquals() to make a test fail, the Maven build is still green. In my opinion there's no need for the DefaultTest, you could make InstrumentedAppenderTest use a custom registry and remove the inner classes.
There was a problem hiding this comment.
Thanks for the review. Good catch.
I reverted the change and did something more simple/less fancy. I added two appenders (one default and one for a custom registry) into the test and then just appended to both of them and checked both of them. That should work.
Happy to revert even this if you would prefer the simplicity of testing only with the custom registry. Just wanted to push it up as an alternative option.
Tested locally by changing an assertEquals(...) this time.
Signed-off-by: Matthew Dolan <MatthewDolan@users.noreply.github.com>
I needed the functionality to create an
InstrumentedAppenderfor logback which registers the metrics with a custom registry (other than the default registry). The code only supported the assumed default registry. This pull request adds a second public constructor for supplying a registry.This pull request also abstracts the tests and runs them twice. Once for the default registry and once for a supplied registry.