Gauges should be registered with registerWithReplacement
#1123
Conversation
Generate changelog in
|
Using TaggedMetricRegistry.gauge is equivalent to map.putIfAbsent, and can result in subtle resource leaks. Prefer replacing existing gauge values using `registerWithReplacement`. This check doesn't apply unless a new enough version of Tritium is available on the compilation classpath.
carterkozak
force-pushed
the
ckozak/UnsafeGaugeRegistration
branch
from
7975232 to
a35150e
Compare
| @BeforeTemplate | ||
| void before(TaggedMetricRegistry registry, MetricName name, Gauge<T> gauge) { | ||
| registry.remove(name); | ||
| registry.registerWithReplacement(name, gauge); |
There was a problem hiding this comment.
Should we have a template to replace following pattern with registry.registerWithReplacement(name, gauge);?
@BeforeTemplate
void before(TaggedMetricRegistry registry, MetricName name, Gauge<T> gauge) {
registry.remove(name);
registry.gauge(name, gauge);
}
There was a problem hiding this comment.
I don't want to create a change that doesn't compile on old versions of Tritium (pre 0.16.1). The error-prone check only runs on new tritium, and transforms registry.gauge(name, gauge) into registry.registerWithReplacement(name, gauge), allowing this rule to do the rest.
Documented in the class level javadoc :-)
| * This implementation is forked from upstream error-prone to work around a bug that results in | ||
| * parameters being renamed in some scenarios: https://github.com/google/error-prone/issues/1451. | ||
| */ | ||
| public static SuggestedFix renameMethodInvocation( |
There was a problem hiding this comment.
Upstream PR: google/error-prone#1452
|
Validated in a large internal project. |
After this PR
==COMMIT_MSG==
Using TaggedMetricRegistry.gauge is equivalent to map.putIfAbsent, and
can result in subtle resource leaks. Prefer replacing existing gauge
values using
registerWithReplacement.This check doesn't apply unless a new enough version of Tritium
is available on the compilation classpath.
==COMMIT_MSG==
Possible downsides?
It's possible some consumers expect subsequent registrations not to replace the existing gauge, however in the scenarios we're aware of, the return value is used in these cases.