Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an InstrumentedAppender constructor that allows passing a CollectorRegistry #690

Merged
merged 2 commits into from
Sep 11, 2021
Merged

Add an InstrumentedAppender constructor that allows passing a CollectorRegistry #690

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
83238c6
Add the InstrumentedAppender constructor that allows passing a Collec…
MatthewDolan Sep 7, 2021
9cb5471
address comment: fix tests
MatthewDolan Sep 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 33 additions & 25 deletions simpleclient_logback/src/main/java/io/prometheus/client/logback/InstrumentedAppender.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,38 +3,46 @@
import ch.qos.logback.classic.Level;
import ch.qos.logback.classic.spi.ILoggingEvent;
import ch.qos.logback.core.UnsynchronizedAppenderBase;
import io.prometheus.client.CollectorRegistry;
import io.prometheus.client.Counter;

public class InstrumentedAppender extends UnsynchronizedAppenderBase<ILoggingEvent> {

public static final String COUNTER_NAME = "logback_appender_total";

private static final Counter COUNTER;
private static final Counter.Child TRACE_LABEL;
private static final Counter.Child DEBUG_LABEL;
private static final Counter.Child INFO_LABEL;
private static final Counter.Child WARN_LABEL;
private static final Counter.Child ERROR_LABEL;

static {
COUNTER = Counter.build().name(COUNTER_NAME)
.help("Logback log statements at various log levels")
.labelNames("level")
.register();

TRACE_LABEL = COUNTER.labels("trace");
DEBUG_LABEL = COUNTER.labels("debug");
INFO_LABEL = COUNTER.labels("info");
WARN_LABEL = COUNTER.labels("warn");
ERROR_LABEL = COUNTER.labels("error");
}
private static final Counter defaultCounter = Counter.build().name(COUNTER_NAME)
.help("Logback log statements at various log levels")
.labelNames("level")
.register();
private final Counter.Child traceCounter;
private final Counter.Child debugCounter;
private final Counter.Child infoCounter;
private final Counter.Child warnCounter;
private final Counter.Child errorCounter;

/**
* Create a new instrumented appender using the default registry.
*/
public InstrumentedAppender() {
this(defaultCounter);
}

/**
* Create a new instrumented appender using the supplied registry.
*/
public InstrumentedAppender(CollectorRegistry registry) {
this(Counter.build().name(COUNTER_NAME)
.help("Logback log statements at various log levels")
.labelNames("level")
.register(registry));
}

private InstrumentedAppender(Counter counter) {
this.traceCounter = counter.labels("trace");
this.debugCounter = counter.labels("debug");
this.infoCounter = counter.labels("info");
this.warnCounter = counter.labels("warn");
this.errorCounter = counter.labels("error");
}

@Override
public void start() {
Expand All @@ -45,19 +53,19 @@ public void start() {
protected void append(ILoggingEvent event) {
switch (event.getLevel().toInt()) {
case Level.TRACE_INT:
TRACE_LABEL.inc();
this.traceCounter.inc();
break;
case Level.DEBUG_INT:
DEBUG_LABEL.inc();
this.debugCounter.inc();
break;
case Level.INFO_INT:
INFO_LABEL.inc();
this.infoCounter.inc();
break;
case Level.WARN_INT:
WARN_LABEL.inc();
this.warnCounter.inc();
break;
case Level.ERROR_INT:
ERROR_LABEL.inc();
this.errorCounter.inc();
break;
default:
break;
Expand Down
103 changes: 63 additions & 40 deletions ...leclient_logback/src/test/java/io/prometheus/client/logback/InstrumentedAppenderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,65 +12,88 @@
import org.junit.Test;

public class InstrumentedAppenderTest {
public abstract static class Base {
CollectorRegistry registry;
InstrumentedAppender appender;
private ILoggingEvent event;

private InstrumentedAppender appender;
private ILoggingEvent event;
@Before
public void setUp() throws Exception {
this.appender.start();

@Before
public void setUp() throws Exception {
appender = new InstrumentedAppender();
appender.start();

event = mock(ILoggingEvent.class);
}
this.event = mock(ILoggingEvent.class);
}

@Test
public void metersTraceEvents() throws Exception {
when(event.getLevel()).thenReturn(Level.TRACE);
@Test
public void metersTraceEvents() throws Exception {
when(this.event.getLevel()).thenReturn(Level.TRACE);

appender.doAppend(event);
this.appender.doAppend(event);

assertEquals(1, getLogLevelCount("trace"));
}
assertEquals(1, this.getLogLevelCount("trace"));
}

@Test
public void metersDebugEvents() throws Exception {
when(event.getLevel()).thenReturn(Level.DEBUG);
@Test
public void metersDebugEvents() throws Exception {
when(this.event.getLevel()).thenReturn(Level.DEBUG);

appender.doAppend(event);
this.appender.doAppend(event);

assertEquals(1, getLogLevelCount("debug"));
}
assertEquals(1, this.getLogLevelCount("debug"));
}

@Test
public void metersInfoEvents() throws Exception {
when(event.getLevel()).thenReturn(Level.INFO);
@Test
public void metersInfoEvents() throws Exception {
when(this.event.getLevel()).thenReturn(Level.INFO);

appender.doAppend(event);
this.appender.doAppend(event);

assertEquals(1, getLogLevelCount("info"));
}
assertEquals(1, this.getLogLevelCount("info"));
}

@Test
public void metersWarnEvents() throws Exception {
when(this.event.getLevel()).thenReturn(Level.WARN);

@Test
public void metersWarnEvents() throws Exception {
when(event.getLevel()).thenReturn(Level.WARN);
this.appender.doAppend(event);

appender.doAppend(event);
assertEquals(1, this.getLogLevelCount("warn"));
}

assertEquals(1, getLogLevelCount("warn"));
@Test
public void metersErrorEvents() throws Exception {
when(this.event.getLevel()).thenReturn(Level.ERROR);

this.appender.doAppend(event);

assertEquals(1, this.getLogLevelCount("error"));
}

private int getLogLevelCount(String level) {
return this.registry.getSampleValue(COUNTER_NAME,
new String[]{"level"}, new String[]{level}).intValue();
}
}

@Test
public void metersErrorEvents() throws Exception {
when(event.getLevel()).thenReturn(Level.ERROR);
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();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}