First step to solve issue 17 #19

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

medecau commented Jan 25, 2013

#17

medecau added some commits Jan 25, 2013

new file to test rules
new utils.py file to hold stuff used in other files  trying to avoid interdependency

**mem limits are not firing alarms**
Owner

sebastien commented Jan 25, 2013

Hi! that's a great start. I'd like to merge it but I'd prefer to wait until you have completed the breaking down and made sure you have backward-compatibility. All you would need to do is to finish the splitting in sub-modules and add a init.py with auto-importing of sub-modules and then it should be ready for merging.

Contributor

rantav commented Jun 1, 2014

Does this commit deprecate this issue?
c646e83

Owner

sebastien commented Jun 2, 2014

Yes, I'm closing this ticket.

@sebastien sebastien closed this Jun 2, 2014

Contributor

rantav commented Jun 2, 2014

How do you feel about going further and splitting the currently large
init file into smaller actions and monitors?
On Jun 2, 2014 4:00 PM, "Sébastien Pierre" notifications@github.com wrote:

Closed #19 #19.


Reply to this email directly or view it on GitHub
#19 (comment).

Owner

sebastien commented Jun 2, 2014

That's a very good idea. I'd like to do the following:

monitoring/core.py -> all the utility function & base classes, plus the main Monitor class
monitoring/actions.py -> all the default actions
monitoring/rules.py -> all the default rules

and then make the __init__.py as lightweight as possible. Any thoughts?

Contributor

rantav commented Jun 3, 2014

yes, I was thinking something along these lines, just with a slight
variation:
Instead of having a single file actions.py and monitors.py etc, create
modules (directories) with the same names and a single file per action or
monitor or rule. Then in the init file just import the monitors,
actions etc so that it's easy to use them externally (e.g. from
monitoring.monitors import Jenkins, from monitoring.actions import Librato)
similar to what I sent in my recent pull request.
I think it will be slightly easier to maintain and grow this way.

On Mon, Jun 2, 2014 at 5:34 PM, Sébastien Pierre notifications@github.com
wrote:

That's a very good idea. I'd like to do the following:

monitoring/core.py -> all the utility function & base classes, plus the
main Monitor class
monitoring/actions.py -> all the default actions
monitoring/rules.py -> all the default rules

and then make the init.py as lightweight as possible. Any thoughts?


Reply to this email directly or view it on GitHub
#19 (comment).

/Ran
gormim.com http://www.gormim.com

Owner

sebastien commented Jun 3, 2014

The problem I see with having one file per actions/rule is having too many small files, as the base actions and rules are not going to be big/complex classes. This could be solved by having the actions.py and rules as monitoring/actions/__init__.py and `monitoring/rules/init.py.

One thing is that monitors are actually rules (some of them might be about monitoring, but not necessarily). I just realized that and renamed monitoring.monitors to monitoring.rules.

Also, I think it's important to ensure that monitoring loads fast and limit the number of dependencies at import. In particular importing monitoring.rules should not import all the rules sub-modules. So you wouldn't be able to do from monitoring.rules import Jenkins but would do from monitoring.rules.jenkins import Jenkins. It's a minor syntactical annoyance that will ensure monitoring has a low footprint.

Contributor

rantav commented Jun 3, 2014

Ok sounds good. I wasn't aware of the concept of rules so I just made up
a name - monitors. Rules sounds just fine :)
Regarding separation to files etc I prefer separation as much as
possible. Maybe it's bc of my Java background... But I'm cool with your
preference to keep some smaller classes on a single file
On Jun 3, 2014 5:12 PM, "Sébastien Pierre" notifications@github.com wrote:

The problem I see with having one file per actions/rule is having too many
small files, as the base actions and rules are not going to be big/complex
classes. This could be solved by having the actions.py and rules as
monitoring/actions/init.py and `monitoring/rules/init.py.

One thing is that monitors are actually rules (some of them might be
about monitoring, but not necessarily). I just realized that and renamed
monitoring.monitors to monitoring.rules.

Also, I think it's important to ensure that monitoring loads fast and
limit the number of dependencies at import. In particular importing
monitoring.rules should not import all the rules sub-modules. So you
wouldn't be able to do from monitoring.rules import Jenkins but would do from
monitoring.rules.jenkins import Jenkins. It's a minor syntactical
annoyance that will ensure monitoring has a low footprint.


Reply to this email directly or view it on GitHub
#19 (comment).

Owner

sebastien commented Jun 3, 2014

Yes, it's definitely more common with Java. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment