[ANN] go-librato and go-metrics
Richard Crowley
tool to the Librato Metrics API [1]. Possibly not of as much interest
as the next one.
<https://github.com/rcrowley/go-metrics>: a Go port of Coda Hale's
Java/Scala Metrics library for capturing application-level metrics
including counters (incrementable/decrementable values), gauges
(instantaneous values), histograms (including min, max, standard
deviation, and percentiles), meters (event rates with
exponentially-weighted moving averages), and timers (histogram +
meter).
Both are pretty green so I'd appreciate folks pointing out things that
could be better.
Richard
[1] <http://dev.librato.com/v1/metrics>
[2] <https://github.com/codahale/metrics>
David Anderson
<https://github.com/rcrowley/go-librato>: a Go client and command-line
tool to the Librato Metrics API [1]. Possibly not of as much interest
as the next one.
<https://github.com/rcrowley/go-metrics>: a Go port of Coda Hale's
Java/Scala Metrics library for capturing application-level metrics
including counters (incrementable/decrementable values), gauges
(instantaneous values), histograms (including min, max, standard
deviation, and percentiles), meters (event rates with
exponentially-weighted moving averages), and timers (histogram +
meter).
Both are pretty green so I'd appreciate folks pointing out things that
could be better.
John Asmuth
The Readme says "This code is not safe on 32-bit architectures." How come?
Richard Crowley
That's close. It's actually because the atomic package is missing a
way to read a 64-bit value atomically on a 32-bit platform. If you
`objdump -d ...` a 32-bit Go program you'll see that it takes two
instructions to copy one int64 to another (for one example). I
believe this would open the possibility of reading half of one value
and half of another.
Richard
Ian Lance Taylor
sync/atomic.LoadInt64 should work atomically on a 32-bit platform. On
32-bit x86 it does it by using MMX instructions. On ARM it uses ldrexd
and strexd.
Ian
Richard Crowley
>> way to read a 64-bit value atomically on a 32-bit platform. If you
>> `objdump -d ...` a 32-bit Go program you'll see that it takes two
>> instructions to copy one int64 to another (for one example). I
>> believe this would open the possibility of reading half of one value
>> and half of another.
>
> sync/atomic.LoadInt64 should work atomically on a 32-bit platform. On
> 32-bit x86 it does it by using MMX instructions. On ARM it uses ldrexd
> and strexd.
>
> Ian
>
gauge.go:24: undefined: atomic.LoadInt64
I see this does exist in bleeding-edge Go:
<http://tip.goneat.org/pkg/sync/atomic/#LoadInt64>. What's the
etiquette here?
Richard
John Asmuth
roger peppe
> <https://github.com/rcrowley/go-librato>: a Go client and command-line
> tool to the Librato Metrics API [1]. Possibly not of as much interest
> as the next one.
>
> <https://github.com/rcrowley/go-metrics>: a Go port of Coda Hale's
> Java/Scala Metrics library for capturing application-level metrics
> including counters (incrementable/decrementable values), gauges
> (instantaneous values), histograms (including min, max, standard
> deviation, and percentiles), meters (event rates with
> exponentially-weighted moving averages), and timers (histogram +
> meter).
>
> Both are pretty green so I'd appreciate folks pointing out things that
> could be better.
Hi Richard,
I've just had a very brief glance at your metrics package - I just
did goinstall github.com/codahale/metrics then godoc.
So these comments are a very shallow first reaction to
the API - please take with a pinch of salt.
- There are no doc comments on any of the functions.
All exported functions and types should have some documentation.
- Int64Slice shouldn't be exported - it's not used anywhere else
in the API.
- it seems that Log and Syslog do very similar things and could
probably be merged (the disparity between log and log/syslog
doesn't make this very easy though, I agree).
- in quite a few places you return interfaces, and it's not
obvious that it wouldn't be just fine to use static types instead
(for example Registry, Healthcheck, Timer)
- Healthcheck would conventionally be spelled HealthCheck
- Registry.Unregister could be implemented by simply calling
Registry.Register("foo", nil)
- Registry.GetCounter etc could simply return the object - if it's not
there, it will be nil;
no need to return the extra bool.
- Rather than have access methods for Counters, Gauges, etc,
Registry could simply expose the fields directly - a caller can mess with
the contents of the map anyway. That said, see the next point.
- There seems to be no provision for thread-safety - Log and Syslog for
example access fields inside a registry with no mutex; if the Registry is
changing, the program could crash. Probably all the various types should
be mutex-guarded, and methods which currently return bare maps
(e.g. Registry.Counters) removed, because there's no way of making
those maps thread-safe.
Hope these notes are of some help.
cheers,
rog.
Richard Crowley
> Hi Richard,
>
> I've just had a very brief glance at your metrics package - I just
> did goinstall github.com/codahale/metrics then godoc.
> So these comments are a very shallow first reaction to
> the API - please take with a pinch of salt.
>
> - There are no doc comments on any of the functions.
> All exported functions and types should have some documentation.
Caught me. I've added most of them.
>
> - Int64Slice shouldn't be exported - it's not used anywhere else
> in the API.
I've made this private.
>
> - it seems that Log and Syslog do very similar things and could
> probably be merged (the disparity between log and log/syslog
> doesn't make this very easy though, I agree).
They are very similar in structure, yes, but the log format and API
are different. This isn't a dig on Go but sometimes it's just easier
to almost duplicate code.
>
> - in quite a few places you return interfaces, and it's not
> obvious that it wouldn't be just fine to use static types instead
> (for example Registry, Healthcheck, Timer)
All of these were conscious decisions. Many objects in many programs
have (some metric)-like behavior which I want to allow registering as
metrics.
>
> - Healthcheck would conventionally be spelled HealthCheck
Probably but I like the parallelism in capitalization with
RegisterHistogram and RegisterHealthcheck.
>
> - Registry.Unregister could be implemented by simply calling
> Registry.Register("foo", nil)
I like the directness of Unregister.
>
> - Registry.GetCounter etc could simply return the object - if it's not
> there, it will be nil;
> no need to return the extra bool.
You're right. Plus, it makes using GetCounter and friends much easier.
>
> - Rather than have access methods for Counters, Gauges, etc,
> Registry could simply expose the fields directly - a caller can mess with
> the contents of the map anyway. That said, see the next point.
Rather than exposing the fields directly I've chosen to add some
callback APIs: EachCounter(func(string, Counter)), etc. I've removed
the accessors and kept the maps private.
>
> - There seems to be no provision for thread-safety - Log and Syslog for
> example access fields inside a registry with no mutex; if the Registry is
> changing, the program could crash. Probably all the various types should
> be mutex-guarded, and methods which currently return bare maps
> (e.g. Registry.Counters) removed, because there's no way of making
> those maps thread-safe.
Caught me again. I spent a lot of time on the concurrency of the
metrics themselves but slacked off here. I've added and appropriately
locked a mutex around registry operations. The callback APIs
mentioned above were adopted so the mutex could be managed without the
caller's knowledge.
>
> Hope these notes are of some help.
>
> cheers,
> rog.
>
Fantastic. Thanks again.
Richard
roger peppe
> Thanks for all your comments, Roger! See answers inline:
>
>> Hi Richard,
>>
>> I've just had a very brief glance at your metrics package - I just
>> did goinstall github.com/codahale/metrics then godoc.
>> So these comments are a very shallow first reaction to
>> the API - please take with a pinch of salt.
>>
>> - There are no doc comments on any of the functions.
>> All exported functions and types should have some documentation.
>
> Caught me. I've added most of them.
cool.
point of information: function and type comments in Go
are conventionally written as complete sentences.
for instance "CaptureRuntimeMemStats captures new values for..."
rather than "Captures new values for...".
>> - it seems that Log and Syslog do very similar things and could
>> probably be merged (the disparity between log and log/syslog
>> doesn't make this very easy though, I agree).
>
> They are very similar in structure, yes, but the log format and API
> are different. This isn't a dig on Go but sometimes it's just easier
> to almost duplicate code.
i agree that it's easier sometimes, but in this case it's fairly
trivial to make them both use the same code.
for example: http://paste.ubuntu.com/746937/
>> - in quite a few places you return interfaces, and it's not
>> obvious that it wouldn't be just fine to use static types instead
>> (for example Registry, Healthcheck, Timer)
>
> All of these were conscious decisions. Many objects in many programs
> have (some metric)-like behavior which I want to allow registering as
> metrics.
i see that there are some useful interfaces there - i'm not saying
that you don't want to define interfaces for the things passed
into Registry for example. but i think that things would
be more obvious if the New functions returned statically defined
types - for instance if NewRegistry returned *StandardRegistry
rather than the interface.
i wonder what kind of advantage you're seeing to having
Registry, in particular, as an interface. as it is, it's just
a repository for information. what other kinds of Registries
do you see being useful?
in fact, thinking about this a bit more, i am not sure that
i see how it is advantageous to have the Registry hold
all those specific types.
it seems to me that the only thing all those kinds of things
that can be registered need to have in common is the
ability to print their information to a log.
why not just something like this:
type Logger interface {
Log(printf func(string, ...interface{}))
}
type Registry struct {
mu sync.Mutex
stats map[string] Logger
}
func (r *Registry) Register(name string, stat Logger)
func (r *Registry) Unregister(name string)
func (r *Registry) Get(name string) Logger
func (r *Registry) Each(func(Logger))
then each useful stats-related New function could just
return its static type (e.g. func NewHistogram()*StandardHistogram),
and if there are some useful interfaces in common they can be abstracted out
independently.
for example:
// writeHistograms writes SVG output to w for all the
// histograms registered with the Registry.
func writeHistograms(r *Registry, w io.Writer) {
r.Each(func(l Logger) {
if h, ok := l.(Histogram); ok {
p := h.Percentile(...)
// etc
}
}
}
by going this way you remove, i think, quite a lot of boilerplate
code without making things any harder to use or less general.
and interfaces are used in the way they work best - to express
commonalities of functionality.