discourse / prometheus_exporter

A framework for collecting and aggregating prometheus metrics
MIT License
525 stars 153 forks source link

Was the Puma memory leak fix intentionally "reverted"? #275

Closed adamk9k closed 1 year ago

adamk9k commented 1 year ago

Hey folks, I was searching for known issues of memory leaks and encountered this discussion from 2019:

https://github.com/discourse/prometheus_exporter/issues/69

the outcome of which was this fix https://github.com/discourse/prometheus_exporter/commit/e2d75a5aa84bc71e8bfcf07d8a406e1145ed9958 for the Puma collector because it never released old objects from memory.

To my surprise, this piece of code is not there in the most recent versions https://github.com/discourse/prometheus_exporter/blob/main/lib/prometheus_exporter/server/puma_collector.rb#L56-L58 and checking the blame the collect method of the Puma collector seems to be unchanged since it was originally added https://github.com/discourse/prometheus_exporter/blame/8db6dc16120492f1bb32a00d66ac5a0d5745c752/lib/prometheus_exporter/server/puma_collector.rb#L56-L58

The fix commit however wasn't completely reverted since I see MAX_PUMA_METRIC_AGE still defined as a constant (was added with the fix) but it's not used anywhere.

Was this intentional or just a mistake? I would be happy to add it back with another PR if it's the latter.

Thanks

SamSaffron commented 1 year ago

Feels like a regressive to me

On Thu, 23 Mar 2023 at 8:18 pm, Adam Kovacs @.***> wrote:

Hey folks, I was searching for known issues of memory leaks and encountered this discussion from 2019:

69 https://github.com/discourse/prometheus_exporter/issues/69

the outcome of which was this fix e2d75a5 https://github.com/discourse/prometheus_exporter/commit/e2d75a5aa84bc71e8bfcf07d8a406e1145ed9958 for the Puma collector because it never released old objects from memory.

To my surprise, this piece of code is not there in the most recent versions https://github.com/discourse/prometheus_exporter/blob/main/lib/prometheus_exporter/server/puma_collector.rb#L56-L58 and checking the blame the collect method of the Puma collector seems to be unchanged since it was originally added https://github.com/discourse/prometheus_exporter/blame/8db6dc16120492f1bb32a00d66ac5a0d5745c752/lib/prometheus_exporter/server/puma_collector.rb#L56-L58

The fix commit however wasn't completely reverted since I see MAX_PUMA_METRIC_AGE still defined as a constant (was added with the fix) but it's not used anywhere.

Was this intentional or just a mistake? I would be happy to add it back with another PR if it's the latter.

Thanks

— Reply to this email directly, view it on GitHub https://github.com/discourse/prometheus_exporter/issues/275, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABIXMV2CA272MVW5C6XZ3W5QINLANCNFSM6AAAAAAWE5HW5E . You are receiving this because you are subscribed to this thread.Message ID: @.***>

adamk9k commented 1 year ago

Ok, I will add a PR to add it back

adamk9k commented 1 year ago

Added as a cherry-pick https://github.com/discourse/prometheus_exporter/pull/276