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

 [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.