Merge lp://staging/~theiw/txstatsd/refactor-with-gaugemetric into lp://staging/txstatsd

[1] I don't see a reason to move service.py into server. Since it uses both
    client and server, it should remain at top-level.

[2] Similarly, I don't see the point of moving the client protocol part to
    client/client.py. Just moving it to client.py would have been enough.

[3] I like that you added a raw socket UdpStatsDClient, but the naming is
    confusing because StatsDClient also uses UDP, though it relies on a twisted
    transport. Since StatsDClient takes an optional 'transport' argument, they
    are not even interchangeable. I'd just remove it unless you plan to use it
    somewhere.

[4] The separation of the Metrics class and the individual Metric classes seems
    fairly arbitrary, and I don't think having a reference to the connection is
    helping much here. I suggest getting rid of the GaugeMetric class and move
    the implementation of 'mark' from GaugeMetric to the 'gauge' method into the
    Metrics class, also move 'send' back to 'Metrics'.

[5] Once you do the above, it makes sense to move metrics/metrics.py back to
    just metrics.py, IMO.

[6] Please keep process.py and report.py top-level, since they have little
    relation to 'server'. In fact, I'm using functions from 'report' on my statsd
    clients, so it's weird to import it from 'server'.

[7] I like that you split 'flush_gauge' out of 'flush'. While you're there,
    maybe it makes sense to refactor the counters and timers handling into
    'flush_counters' and 'flush_timers'.

 [1] I agree given service.py in its current form. However, are
you tempted to separate the client and server responsibilities.

[2] I think it useful to be explicit on namespace.

[3] I did originally have StatsDClient as TwistedStatsDClient. Perhaps
having TwistedStatsDClient and UdpStatsDClient might be
appealing.

[4] Metrics was arrived at after several discussions with Lucio.
I'll let Lucio provide his comments on this.

[5] As for [4].

[6] I don't believe I changed the location of process.py.
report.py - given it is Graphite reporting - I thought belonged
to the txstatsd server; do you see that differently?

[7] I think that would be useful.

many thanks,
ian

On Thursday, 21 July 2011 at 16:13, Sidnei da Silva wrote:

> Review: Needs Fixing
> [1] I don't see a reason to move service.py into server. Since it uses both
> client and server, it should remain at top-level.
>
> [2] Similarly, I don't see the point of moving the client protocol part to
> client/client.py. Just moving it to client.py would have been enough.
>
> [3] I like that you added a raw socket UdpStatsDClient, but the naming is
> confusing because StatsDClient also uses UDP, though it relies on a twisted
> transport. Since StatsDClient takes an optional 'transport' argument, they
> are not even interchangeable. I'd just remove it unless you plan to use it
> somewhere.
>
> [4] The separation of the Metrics class and the individual Metric classes seems
> fairly arbitrary, and I don't think having a reference to the connection is
> helping much here. I suggest getting rid of the GaugeMetric class and move
> the implementation of 'mark' from GaugeMetric to the 'gauge' method into the
> Metrics class, also move 'send' back to 'Metrics'.
>
> [5] Once you do the above, it makes sense to move metrics/metrics.py back to
> just metrics.py, IMO.
>
> [6] Please keep process.py and report.py top-level, since they have little
> relation to 'server'. In fact, I'm using functions from 'report' on my statsd
> clients, so it's weird to import it from 'server'.
>
> [7] I like that you split 'flush_gauge' out of 'flush'. While you're there,
> maybe it makes sense to refactor the counters and timers handling into
> 'flush_counters' and 'flush_timers'.
>
> --
> https://code.launchpad.net/~theiw/txstatsd/refactor-with-gaugemetric/+merge/68683
> You are the owner of lp:~theiw/txstatsd/refactor-with-gaugemetric.

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.

 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.

1- ack
2- i would prefer removing now. but this is sidnei's call.
3- I am not saying i object to that because of memory. I am saying that classes on the client side and just methods on the server seems unbalanced. And although client side calculations might be relevant, the current patter is "message txstatsd when something happens, do the calculations on txstatsd, txstatsd reports to graphite periodically.". Changing that would be a different branch, a different argument.

On Fri, Jul 22, 2011 at 11:14 AM, Lucio Torre <email address hidden> wrote:
> 2- i would prefer removing now. but this is sidnei's call.

I'm fine with removing the old Meter stuff now.

--
Sidnei

Make the most of Ubuntu with Landscape
http://landscape.canonical.com

Let me attempt a summary of the comments:

[1] service.py to be placed at top-level.

[2] client.py to remain in txstatsd/client.

[3] Rename StatsDClient to TwistedStatsDClient. UdpStatsDClient to remain.

[4] metrics.py to remain as it is, and future revisions will
introduce the additional concrete metrics classes.

[5] metrics.py to remain in txstatsd/metrics. Given the introduction
of future metric classes, I think there will be benefit from having
the separate namespace.

[6] process.py is already top-level. I'll move report.py to top-level.
I do believe it useful to separate server and client behaviour.

[7] processor.py to be refactored following the style introduced
with gauge metric handling.

[8] meter.py to be removed.

I will make the above changes.

On Friday, 22 July 2011 at 15:36, Sidnei da Silva wrote:

> On Fri, Jul 22, 2011 at 11:14 AM, Lucio Torre <<email address hidden> (mailto:<email address hidden>)> wrote:
> > 2- i would prefer removing now. but this is sidnei's call.
>
> I'm fine with removing the old Meter stuff now.
>
> --
> Sidnei
>
> Make the most of Ubuntu with Landscape
> http://landscape.canonical.com
>
> https://code.launchpad.net/~theiw/txstatsd/refactor-with-gaugemetric/+merge/68683
> You are the owner of lp:~theiw/txstatsd/refactor-with-gaugemetric.

On Fri, Jul 22, 2011 at 12:40 PM, Ian Wilkinson
<email address hidden> wrote:
> [2] client.py to remain in txstatsd/client.

I'd put it at txstatsd/client.py, since there shouldn't be any more
client stuff that doesn't fit in a single module?

-- Sidnei

 If we have txstatsd/server, it seems reasonable to have txstatsd/client.

But...I'll place it in txstatsd/client.py.

On Friday, 22 July 2011 at 16:57, Sidnei da Silva wrote:

> On Fri, Jul 22, 2011 at 12:40 PM, Ian Wilkinson
> <<email address hidden> (mailto:<email address hidden>)> wrote:
> > [2] client.py to remain in txstatsd/client.
>
> I'd put it at txstatsd/client.py, since there shouldn't be any more
> client stuff that doesn't fit in a single module?
>
> -- Sidnei
>
> --
> https://code.launchpad.net/~theiw/txstatsd/refactor-with-gaugemetric/+merge/68683
> You are the owner of lp:~theiw/txstatsd/refactor-with-gaugemetric.