adutra commented on code in PR #1916:
URL:
https://github.com/apache/cassandra-java-driver/pull/1916#discussion_r1532412536
##########
metrics/micrometer/src/main/java/com/datastax/oss/driver/internal/metrics/micrometer/MicrometerMetricUpdater.java:
##########
@@ -100,7 +100,10 @@ protected void initializeGauge(
m -> {
MetricId id = getMetricId(m);
Iterable<Tag> tags = MicrometerTags.toMicrometerTags(id.getTags());
- return Gauge.builder(id.getName(),
supplier).tags(tags).register(registry);
+ return Gauge.builder(id.getName(), supplier)
+ .strongReference(false)
Review Comment:
This is really clever, but I wonder if we aren't masking a broader problem:
When a session is closed, ideally, I'd like all metrics (session- and
node-level ones) that were registered by it to be explicitly de-registered.
This is not the case currently, and that's imho the real cause why the memory
leak is happening in the first place.
Explicitly de-registering metrics is especially welcome if the sessions are
sharing the same registry, which is the case by default when using Micrometer.
If the registry is not shared, I bet there is no memory leak, but still, it'd
be more correct to de-register everything.
My problem with the fix proposed here is that it will fix the issue for
Micrometer gauges, but what if MicroProfile or Dropwizard change something in
the future that would cause yet another strong reference to be retained by a
metric?
I wonder if we shouldn't investigate introducing a new method `void
deregisterMetrics()` in `MetricUpdater`.
Then we could modify `DefaultSession` and add this to
`DefaultSession.close()` and `forceClose()`:
```java
metricUpdater.deregisterMetrics();
for (Node n : metadataManager.getMetadata().getNodes().values()) {
((DefaultNode) n).getMetricUpdater().deregisterMetrics();
}
```
The implementation for `MicrometerMetricUpdater` should be trivial (and for
other impls too, I guess):
```java
@Override
public void deregisterMetrics() {
metrics.values().forEach(registry::remove);
}
```
But obviously, the devil is likely hiding in the details :-)
I'd be curious to hear what others think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
