always enable the MonitoringProducer

[mikebonnet]

moksha records the time it takes a Consumer to process every message in the Consumer._times list. The MonitoringProducer sends those times to the monitor socket, and resets the list. If the MonitoringProducer isn't running, that list grows forever, causing a memory leak. The default configuration should always enable the MonitoringProducer.

[codecov[bot]]

Codecov Report

Merging #515 into develop will not change coverage. The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop     #515   +/-   ##
========================================
  Coverage    60.17%   60.17%           
========================================
  Files           29       29           
  Lines         1750     1750           
  Branches       272      272           
========================================
  Hits          1053     1053           
  Misses         620      620           
  Partials        77       77

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3854e4c...eb630a3. Read the comment docs.

[mikebonnet]

@jeremycline @pypingou @ralphbean Mind taking a look?

[mikebonnet]

@jeremycline I thought about fixing this in moksha, but that would require introspecting the hub and looking for a producer which is an instance of MonitoringProducer, which seemed expensive and racy. Two lines of config looked preferable. I'm fine moving them to config.py if that's a better place for them.

[jeremycline]

You're not actually fixing the problem, though, just hiding it behind a configuration default (a configuration, I might add, for a different project) that someone might change. moksha should not be dumping stuff in a list forever and ever hoping something is removing them. Make it a bounded queue and just drop times if you don't want to deal with the introspection.

[mikebonnet]

I think you could argue that this is a configuration problem in fedmsg. The design of moksha essentially requires that something reads/resets those stats at regular intervals, but the default configuration bundled with fedmsg does not enable MonitoringProducer (or something else that performs a similar function).

[jeremycline]

No, I don't think that's something you could reasonably argue. For one thing, moksha doesn't state that that is a requirement at all. The library has a list in a private variable of one of its classes that, in a perfectly valid configuration*, leaks memory. That's a bug in moksha. It may be the design of moksha, but it's also wrong. If we added it as a default configuration option fedmsg would also be wrong because we'd let a user configure it such that it leaks memory. Let's fix it where it's broken.

*I assume it's valid, although it's not documented anywhere that you must set this configuration, or that it even is a configuration

[mikebonnet]

@jeremycline Fair enough, see https://github.com/mokshaproject/moksha/pull/57