Code review comment for lp://staging/~theiw/txstatsd/refactor-with-gaugemetric

Revision history for this message
Ian Wilkinson (theiw) wrote :

 1 - I originally had https://pastebin.canonical.com/50104/, and in our
discussions, you wanted to limit the introduction of new classes to
a single metric. If everyone is comfortable on approach of metrics.py
then the other metric classes can be introduced.

2 - Meter is there purely for historical reasons. It should be deprecated in a future
(or this) release.

4. I'm still not persuaded by the memory argument, and client-side calculation still
seems relevant.

On Thursday, 21 July 2011 at 21:12, Lucio Torre wrote:

> 1- the code that has metric specific knowledge should be moved out of Metrics into each metric
> For example: self._metrics[name].send("%s|c" % value)
> the "|c" part is too specific to the counter.
>
> 2- there are two classes that seem to do the same thing: Metrics and Meter, but one has no support for gauges.
>
> 3- its very hard to do reviews when most files have been moved around and changed as everything looks new.
>
> 4- my only objection on having specific instances handle each metric on the client side would be memory. Still, having classes on the client to encode each kind of metric on just methods on the server seems unbalanced. Specially since we try to keep all the important metric calculations on the server side.
>
> 5- i agree that the metric code and the transport should be separated.
>
>
> --
> https://code.launchpad.net/~theiw/txstatsd/refactor-with-gaugemetric/+merge/68683
> You are the owner of lp:~theiw/txstatsd/refactor-with-gaugemetric.

« Back to merge proposal