Merge into trunk : add-config-file : Code : txStatsD

Status:	Merged
Approved by:	Sidnei da Silva on 2011-07-04
Approved revision:	13
Merge reported by:	Lucio Torre
Merged at revision:	not available
Proposed branch:	lp://staging/~lucio.torre/txstatsd/add-config-file
Merge into:	lp://staging/txstatsd
Diff against target:	374 lines (+261/-15) 9 files modified MANIFEST.in (+2/-2) setup.py (+16/-1) statsd.tac (+5/-9) twisted/plugins/txstatsd_plugin.py (+22/-0) txstatsd.conf-example (+6/-0) txstatsd/metrics.py (+1/-1) txstatsd/processor.py (+3/-2) txstatsd/service.py (+103/-0) txstatsd/tests/test_service.py (+103/-0)
To merge this branch:	bzr merge lp://staging/~lucio.torre/txstatsd/add-config-file
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Sidnei da Silva		2011-07-04	Approve on 2011-07-04
Review via email: mp+66840@code.staging.launchpad.net

Commit message

add config file support

Description of the change

Adds config file support to txstatsd.

Now you can configure txstatsd using command line arguments and/or a config file when running as a twistd plugin.

After installing, you can now do:
$ twistd txstatsd

And it will start txstatsd with the default parameters.

If --config/-c FILE is specified, it will read the config from that file. All the arguments can also be specified from the command line and will take precedence over the ones in the config file.

Revision history for this message

Sidnei da Silva (sidnei) wrote on 2011-07-04:

#

Download full text (4.7 KiB)

Looks great, with some whitespace cleanup and naming consistency
issues fixed. +1.

[1] pep8 isn't supper happy. :)

=== pep8 ===

txstatsd/tests/test_service.py:15:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:19:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:26:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:30:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:37:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:38:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:39:9: E303 too many blank lines (2)
txstatsd/tests/test_service.py:40:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:41:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:42:5: E303 too many blank lines (2)
txstatsd/tests/test_service.py:47:55: E202 whitespace before ']'
txstatsd/tests/test_service.py:49:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:59:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:63:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:71:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:79:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:85:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:90:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:91:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:96:61: W292 no newline at end of file
twisted/plugins/txstatsd_plugin.py:22:38: W292 no newline at end of file
txstatsd/service.py:12:1: E302 expected 2 blank lines, found 1
txstatsd/service.py:16:1: W293 blank line contains whitespace
txstatsd/service.py:18:1: W293 blank line contains whitespace
txstatsd/service.py:24:1: W293 blank line contains whitespace
txstatsd/service.py:30:1: W293 blank line contains whitespace
txstatsd/service.py:36:1: W293 blank line contains whitespace
txstatsd/service.py:38:1: W293 blank line contains whitespace
txstatsd/service.py:43:1: W293 blank line contains whitespace
txstatsd/service.py:48:1: W293 blank line contains whitespace
txstatsd/service.py:58:1: W293 blank line contains whitespace
txstatsd/service.py:60:1: W293 blank line contains whitespace
txstatsd/service.py:61:1: W293 blank line contains whitespace
txstatsd/service.py:76:1: W293 blank line contains whitespace
txstatsd/service.py:77:1: W293 blank line contains whitespace
txstatsd/service.py:80:1: W293 blank line contains whitespace
txstatsd/service.py:94:1: W293 blank line contains whitespace
txstatsd/service.py:95:19: W292 no newline at end of file
setup.py:30:1: E302 expected 2 blank lines, found 1

[2] some warnings from pyflakes but you can ignore those.

=== pyflakes ===

setup.py:11: 'setuptools' imported but unused
setup.py:14: redefinition of unused 'find_packages' from line 12

[3] is 'refresh_plugin_cache' needed in setup.py? A docstring
explaining why would be nice.

[4] Please use assertEqual(expected, got) instead of assertEquals(got, expected)

[5] The docstring for test_no_config_option is the same as
test_set_parameter, please update...

Looks great, with some whitespace cleanup and naming consistency
issues fixed. +1.

[1] pep8 isn't supper happy. :)

=== pep8 ===

txstatsd/tests/test_service.py:15:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:19:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:26:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:30:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:37:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:38:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:39:9: E303 too many blank lines (2)
txstatsd/tests/test_service.py:40:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:41:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:42:5: E303 too many blank lines (2)
txstatsd/tests/test_service.py:47:55: E202 whitespace before ']'
txstatsd/tests/test_service.py:49:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:59:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:63:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:71:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:79:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:85:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:90:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:91:1: W293 blank line contains whitespace
txstatsd/tests/test_service.py:96:61: W292 no newline at end of file
twisted/plugins/txstatsd_plugin.py:22:38: W292 no newline at end of file
txstatsd/service.py:12:1: E302 expected 2 blank lines, found 1
txstatsd/service.py:16:1: W293 blank line contains whitespace
txstatsd/service.py:18:1: W293 blank line contains whitespace
txstatsd/service.py:24:1: W293 blank line contains whitespace
txstatsd/service.py:30:1: W293 blank line contains whitespace
txstatsd/service.py:36:1: W293 blank line contains whitespace
txstatsd/service.py:38:1: W293 blank line contains whitespace
txstatsd/service.py:43:1: W293 blank line contains whitespace
txstatsd/service.py:48:1: W293 blank line contains whitespace
txstatsd/service.py:58:1: W293 blank line contains whitespace
txstatsd/service.py:60:1: W293 blank line contains whitespace
txstatsd/service.py:61:1: W293 blank line contains whitespace
txstatsd/service.py:76:1: W293 blank line contains whitespace
txstatsd/service.py:77:1: W293 blank line contains whitespace
txstatsd/service.py:80:1: W293 blank line contains whitespace
txstatsd/service.py:94:1: W293 blank line contains whitespace
txstatsd/service.py:95:19: W292 no newline at end of file
setup.py:30:1: E302 expected 2 blank lines, found 1

[2] some warnings from pyflakes but you can ignore those.

=== pyflakes ===

setup.py:11: 'setuptools' imported but unused
setup.py:14: redefinition of unused 'find_packages' from line 12

[3] is 'refresh_plugin_cache' needed in setup.py? A docstring
explaining why would be nice.

[4] Please use assertEqual(expected, got) instead of assertEquals(got, expected)

[5] The docstring for test_no_config_option is the same as
test_set_parameter, please update it. Same in test_reads_from_config,
test_cmdline_overrides_config,

[6] We should decide between 'Statsd' and 'StatsD'. Some of the new
code you added uses the former, while the existing code uses the
latter. I think we should go with the latter.

[7] 'TxStatsdServiceMaker' could be named 'StatsDServiceMaker' and the
tapname could be simply 'statsd'. Since this requires 'twistd' to run,
it's pretty implicit that it's tx-based, so we might as well omit the
'tx' prefix.

[8] Is it really necessary to reach out to __class__ here? Seems like
a workaround for a bug. Either way it needs a comment explaining why.

+    optParameters = []
+
+    def __init__(self):
+        self.glue_defaults = {}
+        self.__class__.optParameters = []

[9] s/setting/settings/
     s/txstatsd/txStatsD/

+    The set of configuration setting for txstatsd.

[10] s/statsd/txStatsD?

+    """Create a statsd service."""

[11] s/txstatsd/statsd?

+    service.setName("txstatsd")

[12] Indentation seems to be off here:

+    listener = UDPServer(options["listen-port"],
+              StatsDServerProtocol(processor))

[13] Could you rename 'gparam' to 'glue_parameters' here please?

+    def get_file_parser(self, gparam=None, **kwargs):

[14] Please add docstrings to test_support_default_not_in_config,
test_ensure_config_values_coerced and test_service.

[15] I suggest using the service name as the section name in the
configuration file, by having serviceName = 'statsd' as a class
attribute in OptionsGlue, and then using 'self.serviceName' instead of
the hardcoded 'main' below:

+                result = self._config_file.get("main", item)

Revision history for this message

Sidnei da Silva (sidnei) on 2011-07-04:

#

review: Approve

lp://staging/~lucio.torre/txstatsd/add-config-file updated on 2011-07-05

14. By Lucio Torre on 2011-07-05: fixes and cleanup

txStatsD

Merge lp://staging/~lucio.torre/txstatsd/add-config-file into lp://staging/txstatsd

Commit message

Description of the change

Preview Diff

Subscribers