Code review comment for lp:~sidnei/txstatsd/platform-and-process-stats

On Mon, Jul 11, 2011 at 9:48 PM, Lucio Torre <email address hidden> wrote:
> 1. The metric prefixes have to end with ".", which seems error prone. The code should add that.

The idea was that by not adding the '.' in the code, someone could use
an empty prefix, but that use-case is much more unlikely I guess.
Changed it to forcibly add '.'.

> 2. To report the metrics you are using counters, i know we need gauges and we dont have them, but incrementing because someone else divides later seems like we are leaking details.

Refactored a bit to make it easy to change from counters to gauges
when they are added (by passing a report_function around).

> 3. Changing the interface of StatsDClientProtocol to take a function that is executed as an argument to report_stats that reads a file and then calls the target function seems like too much coupling for this specific use case.

Removed that and created a ReportingService that keeps track of a list
of LoopingCall objects instead, with a schedule(function, interval,
report_function) method.

> 4. socket.gethostname() will not work as metrics prefix on EC2, where the hostname is the internal ip and might change after rollouts.

Indeed. I guess it'd be good practice to set a proper hostname once
you get the instance up. Anyway, made the prefix configurable through
the plugin by adding a --prefix argument.