Graylog2 / Graylog2/graylog2-server

Cache per-input metrics in DecodingProcessor

Open
#25,332 1 comment 0 reactions 0 assignees View on GitHub
improvement triaged
Dominant language
Java
Stars
8.1k
Forks
1.1k
Avg merge
1d 20h
Merged PRs (30d)
217

Description

## Problem

`DecodingProcessor` performs repeated `MetricRegistry` string building and
`ConcurrentHashMap` lookups on **every message** in the Disruptor hot path.

On each invocation of `postProcessMessage()` the following calls happen:

```java
metricRegistry.meter(name(baseMetricName, "incomplete")).mark(); // error path
metricRegistry.timer(name(baseMetricName, "parseTime")).update(…); // every message
metricRegistry.meter(name(baseMetricName, "processedMessages")).mark(); // every message
metricRegistry.meter(name(baseMetricName, "failures")).mark(); // error path
```

Each `name(baseMetricName, suffix)` call allocates a new `String` (plus a
varargs `String[]`). Each `meter()`/`timer()` call does a `ConcurrentHashMap.get()` inside the `MetricRegistry`.

`baseMetricName` is derived from `(codec.getClass(), inputIdOnCurrentNode)`. Since a given input always uses the same codec class, the set of metric keys is fixed per input and small in number.

## Proposed fix

Cache the `Meter` and `Timer` references per input ID so that the string building and map lookups happen only once per input, not once per message.

A simple approach:

```java
private record InputMetrics(Timer parseTime, Meter processedMessages,
Meter incomplete, Meter failures) {}

private final ConcurrentHashMap inputMetricsCache =
new ConcurrentHashMap<>();

private InputMetrics getInputMetrics(String baseMetricName) {
return inputMetricsCache.computeIfAbsent(baseMetricName, key ->
new InputMetrics(
metricRegistry.timer(name(key, "parseTime")),
metricRegistry.meter(name(key, "processedMessages")),
metricRegistry.meter(name(key, "incomplete")),
metricRegistry.meter(name(key, "failures"))
));
}
```

Then replace all `metricRegistry.meter(name(baseMetricName, …))` calls with cached field accesses, e.g. `metrics.processedMessages().mark()`.

## Impact

- Eliminates 2+ `String` allocations and 2+ `ConcurrentHashMap` lookups per message on the happy path.
- The cache grows only with the number of distinct inputs (typically small).

## Considerations

- `DecodingProcessor` instances are created per Disruptor worker via the `Factory` interface. The cache should be per-instance (instance field), not static, to avoid cross-worker coordination.
- If inputs can be removed at runtime, consider whether stale entries in the cache need eviction. Since `MetricRegistry` itself retains the metrics, holding references in this cache does not change the lifecycle.
- Other processors in the codebase (e.g. `ProcessBufferProcessor`) already cache their metrics in the constructor — this follows the same pattern, just lazily per input.

## Files

- `graylog2-server/src/main/java/org/graylog2/shared/buffers/processors/DecodingProcessor.java`

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.