Skip to content

Commit

Permalink
Fix a bug where duplicate keys are registered in client metrics (#5466)
Browse files Browse the repository at this point in the history
Motivation:

An `IllegalArgumentException` is raised when
`ConnectionPoolListener.metricCollecting()` is used to monitor
connection pools. The changes in #5288 caused the problem. The different
types (`Gauge`, `Counter`) of meters are registered with the same name.
Unfortunately, it wasn't caught by unit tests because the restriction
only exists in `PrometheusMeterRegistry`. `SimpleMeterRegistry` was used
to verify the code.
```java
java.lang.IllegalArgumentException: Failed to register Collector of type MicrometerCollector: armeria_client_connections is already in use by another Collector of type MicrometerCollector
	at io.prometheus.client.CollectorRegistry.register(CollectorRegistry.java:57)
	at io.prometheus.client.Collector.register(Collector.java:307)
	at io.micrometer.prometheus.PrometheusMeterRegistry.lambda$applyToCollector$17(PrometheusMeterRegistry.java:557)
	at java.base/java.util.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1947)
	at io.micrometer.prometheus.PrometheusMeterRegistry.applyToCollector(PrometheusMeterRegistry.java:552)
	at io.micrometer.prometheus.PrometheusMeterRegistry.newGauge(PrometheusMeterRegistry.java:327)
        ...
	at io.micrometer.core.instrument.MeterRegistry.gauge(MeterRegistry.java:311)
	at io.micrometer.core.instrument.Gauge$Builder.register(Gauge.java:195)
	at com.linecorp.armeria.client.ConnectionPoolMetrics$Meters.<init>(ConnectionPoolMetrics.java:118)
	at com.linecorp.armeria.client.ConnectionPoolMetrics.lambda$increaseConnOpened$0(ConnectionPoolMetrics.java:61)
```

Modifications:

- Infix `active` to the meter name and remove `status=active` tag.
- `<prefix>.active.connections` now becomes the meter name for active
connections.
- `.connections` suffix is added to connection pool metrics.

Result:

- You no longer see `IllegalArgumentException` when using
`ConnectionPoolListener.metricCollecting()`.
- Breaking) `.connections` suffix is now automatically added to
connection pool metrics. If you use a custom `MeterIdPrefix` with
`ConnectionPoolListener.metricCollecting(MeterRegistry,MeterIdPrefix)`,
the meter name will change.
  • Loading branch information
ikhoon authored Feb 21, 2024
1 parent 0efc776 commit 0b20455
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,14 @@ static ConnectionPoolListener logging(Ticker ticker) {
* <td>The number of closed connections.</td>
* </tr>
* <tr>
* <td>{@code armeria.client.connections#value{state="active"}}</td>
* <td>{@code armeria.client.active.connections#value}</td>
* <td>The number of active connections.</td>
* </tr>
* </table>
*/
@UnstableApi
static ConnectionPoolListener metricCollecting(MeterRegistry registry) {
return metricCollecting(registry, new MeterIdPrefix("armeria.client.connections"));
return metricCollecting(registry, new MeterIdPrefix("armeria.client"));
}

/**
Expand All @@ -92,15 +92,15 @@ static ConnectionPoolListener metricCollecting(MeterRegistry registry) {
* <th>description</th>
* </tr>
* <tr>
* <td>{@code <name>#count{state="opened"}}</td>
* <td>{@code <name>.connections#count{state="opened"}}</td>
* <td>The number of opened connection.</td>
* </tr>
* <tr>
* <td>{@code <name>#count{state="closed"}}</td>
* <td>{@code <name>.connections#count{state="closed"}}</td>
* <td>The number of closed connections.</td>
* </tr>
* <tr>
* <td>{@code <name>#value{state="active"}}</td>
* <td>{@code <name>.active.connections#value}</td>
* <td>The number of active connections.</td>
* </tr>
* </table>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,17 +104,16 @@ private static final class Meters {
private int activeConnections;

Meters(MeterIdPrefix idPrefix, List<Tag> commonTags, MeterRegistry registry) {
opened = Counter.builder(idPrefix.name())
opened = Counter.builder(idPrefix.name("connections"))
.tags(commonTags)
.tag(STATE, "opened")
.register(registry);
closed = Counter.builder(idPrefix.name())
closed = Counter.builder(idPrefix.name("connections"))
.tags(commonTags)
.tag(STATE, "closed")
.register(registry);
active = Gauge.builder(idPrefix.name(), this, Meters::activeConnections)
active = Gauge.builder(idPrefix.name("active.connections"), this, Meters::activeConnections)
.tags(commonTags)
.tag(STATE, "active")
.register(registry);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,9 @@

import com.linecorp.armeria.common.SessionProtocol;
import com.linecorp.armeria.common.metric.MoreMeters;
import com.linecorp.armeria.common.metric.PrometheusMeterRegistries;

import io.micrometer.core.instrument.MeterRegistry;
import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
import io.netty.util.AttributeMap;
import io.netty.util.DefaultAttributeMap;

Expand All @@ -36,7 +36,8 @@ class ConnectionPoolCollectingMetricTest {

@BeforeEach
void setUp() {
registry = new SimpleMeterRegistry();
// PrometheusMeterRegistry is preferred for testing because it has additional validation.
registry = PrometheusMeterRegistries.newRegistry();
connectionPoolListener = ConnectionPoolListener.metricCollecting(registry);
}

Expand All @@ -49,14 +50,12 @@ void shouldCollectConnectionPoolEvents() throws Exception {
"protocol=H1,remote.ip=10.10.10.10,state=opened}";
final String closedABMetricKey = "armeria.client.connections#count{local.ip=10.10.10.11," +
"protocol=H1,remote.ip=10.10.10.10,state=closed}";
final String activeABMetricKey = "armeria.client.connections#value{local.ip=10.10.10.11," +
"protocol=H1,remote.ip=10.10.10.10,state=active}";
final String activeABMetricKey = "armeria.client.active.connections#value{local.ip=10.10.10.11," +
"protocol=H1,remote.ip=10.10.10.10}";
final String openBAMetricKey = "armeria.client.connections#count{local.ip=10.10.10.10," +
"protocol=H1,remote.ip=10.10.10.11,state=opened}";
final String closedBAMetricKey = "armeria.client.connections#count{local.ip=10.10.10.10," +
"protocol=H1,remote.ip=10.10.10.11,state=closed}";
final String activeBAMetricKey = "armeria.client.connections#value{local.ip=10.10.10.10," +
"protocol=H1,remote.ip=10.10.10.11,state=active}";
final String activeBAMetricKey = "armeria.client.active.connections#value{local.ip=10.10.10.10," +
"protocol=H1,remote.ip=10.10.10.11}";

final AttributeMap attributeMap = new DefaultAttributeMap();

Expand Down

0 comments on commit 0b20455

Please sign in to comment.