New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add riemann package to report metrics to riemann #71

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@pingles

pingles commented Sep 2, 2014

hi,

We use riemann and so have added a package to report events to there. The code can't be immediately pulled in without changing the import to reference rcrowley/go-metrics rather than pingles/go-metrics but that should be it.

Hope it's useful,
Paul @ uSwitch

r.Each(func(name string, i interface{}) {
switch metric := i.(type) {
case metrics.Gauge:
e := &raidman.Event{

This comment has been minimized.

@jpfuentes2

jpfuentes2 Oct 3, 2014

Why not append the events to a slice and send them all with a for at the end much like the influxDB sink does? It would greatly reduce duplication.

@jpfuentes2

jpfuentes2 Oct 3, 2014

Why not append the events to a slice and send them all with a for at the end much like the influxDB sink does? It would greatly reduce duplication.

func counterEvent(name string, count int64) *raidman.Event {
return &raidman.Event{
Host: "", // empty string is converted to os.Hostname() by raidman

This comment has been minimized.

@jpfuentes2

jpfuentes2 Oct 3, 2014

Don't need to specify Host since the zero-value will be "".

@jpfuentes2

jpfuentes2 Oct 3, 2014

Don't need to specify Host since the zero-value will be "".

"fmt"
"github.com/amir/raidman"
"github.com/cenkalti/backoff"
"github.com/pingles/go-metrics"

This comment has been minimized.

@jpfuentes2

jpfuentes2 Oct 3, 2014

This needs to be changed back to github.com/rcrowley/go-metrics

@jpfuentes2

jpfuentes2 Oct 3, 2014

This needs to be changed back to github.com/rcrowley/go-metrics

@mihasya

This comment has been minimized.

Show comment
Hide comment
@mihasya

mihasya Oct 3, 2014

Collaborator

I know I'm gonna come across as a huge hippo

(I'm gonna preserve that initial accidental post for posterity because it's hilarious; went to switch tabs because I knew I wasn't spelling that word correctly at all and hit enter instead of ])

I'm gonna come across as a huge HYPOCRITE being the author of the librato integration, BUT having since encountered the effect these integrations have on the dependency graph, I would recommend this be created as a separate project. In fact, I'm pondering moving the librato stuff into its own repo as well and starting to log a big ole' DEPRECATION notice when people use the integration that's in this repo.

If you use any dependency management tool (which you should if you want repeatable builds, and we do), every time one of these is added (librato, influxdb, etc), everyone that ISN'T using that particular sink has to add it as a dependency anyway.

Collaborator

mihasya commented Oct 3, 2014

I know I'm gonna come across as a huge hippo

(I'm gonna preserve that initial accidental post for posterity because it's hilarious; went to switch tabs because I knew I wasn't spelling that word correctly at all and hit enter instead of ])

I'm gonna come across as a huge HYPOCRITE being the author of the librato integration, BUT having since encountered the effect these integrations have on the dependency graph, I would recommend this be created as a separate project. In fact, I'm pondering moving the librato stuff into its own repo as well and starting to log a big ole' DEPRECATION notice when people use the integration that's in this repo.

If you use any dependency management tool (which you should if you want repeatable builds, and we do), every time one of these is added (librato, influxdb, etc), everyone that ISN'T using that particular sink has to add it as a dependency anyway.

@jpfuentes2

This comment has been minimized.

Show comment
Hide comment
@jpfuentes2

jpfuentes2 Oct 3, 2014

I'd love to see this accepted and will gladly continue helping w/ review or modifying code, if necessary, as we use Riemann at work as well. I'm currently using the Graphite emitter because our Riemann instance forwards them so we can inspect their stream first.

jpfuentes2 commented Oct 3, 2014

I'd love to see this accepted and will gladly continue helping w/ review or modifying code, if necessary, as we use Riemann at work as well. I'm currently using the Graphite emitter because our Riemann instance forwards them so we can inspect their stream first.

@jpfuentes2

This comment has been minimized.

Show comment
Hide comment
@jpfuentes2

jpfuentes2 commented Oct 3, 2014

@jpfuentes2

This comment has been minimized.

Show comment
Hide comment
@jpfuentes2

jpfuentes2 Oct 3, 2014

I think the concern is completely legitimate. @pingles, do you have the bandwidth to rip this out and move to a repo so that others (like myself) can use it?

jpfuentes2 commented Oct 3, 2014

I think the concern is completely legitimate. @pingles, do you have the bandwidth to rip this out and move to a repo so that others (like myself) can use it?

@pingles

This comment has been minimized.

Show comment
Hide comment
@pingles

pingles Oct 4, 2014

I'm on holiday at the moment but happy to do out when I get back, probably
in about 2 weeks. Agree with all the suggestions.

On Friday, October 3, 2014, Jacques Fuentes notifications@github.com
wrote:

I think the concern is completely legitimate. @pingles
https://github.com/pingles, do you have the bandwidth to rip this out
and move to a repo so that others (like myself) can use it?


Reply to this email directly or view it on GitHub
#71 (comment).

pingles commented Oct 4, 2014

I'm on holiday at the moment but happy to do out when I get back, probably
in about 2 weeks. Agree with all the suggestions.

On Friday, October 3, 2014, Jacques Fuentes notifications@github.com
wrote:

I think the concern is completely legitimate. @pingles
https://github.com/pingles, do you have the bandwidth to rip this out
and move to a repo so that others (like myself) can use it?


Reply to this email directly or view it on GitHub
#71 (comment).

@rcrowley

This comment has been minimized.

Show comment
Hide comment
@rcrowley

rcrowley Oct 4, 2014

Owner

OK, you guys are quite the adults, having a reasonable discussion about all of this and coming to a consensus. Can you write all our software?

When you've got it, let me know where to link for the Riemann output.

Owner

rcrowley commented Oct 4, 2014

OK, you guys are quite the adults, having a reasonable discussion about all of this and coming to a consensus. Can you write all our software?

When you've got it, let me know where to link for the Riemann output.

@jpfuentes2

This comment has been minimized.

Show comment
Hide comment
@jpfuentes2

jpfuentes2 Oct 5, 2014

While I have your attention: what do you think about decoupling the time.Tick from the metric sinks (log/librato/etc) from the function which emits the metrics?

For instance, log is like this:

go metrics.Log(metrics.DefaultRegistry, time.Second*5, log.New(os.Stdout, "metrics: ", log.Lmicroseconds))

I want to have private control over the scheduling and possibly do things with the data (such as debugging) before emitting the metrics to the backend sink.

Is this something you'd consider or should I do some copy pasting in my own project to decouple the time.Tick scheduling from the emitter?

jpfuentes2 commented Oct 5, 2014

While I have your attention: what do you think about decoupling the time.Tick from the metric sinks (log/librato/etc) from the function which emits the metrics?

For instance, log is like this:

go metrics.Log(metrics.DefaultRegistry, time.Second*5, log.New(os.Stdout, "metrics: ", log.Lmicroseconds))

I want to have private control over the scheduling and possibly do things with the data (such as debugging) before emitting the metrics to the backend sink.

Is this something you'd consider or should I do some copy pasting in my own project to decouple the time.Tick scheduling from the emitter?

@rcrowley

This comment has been minimized.

Show comment
Hide comment
@rcrowley

rcrowley Oct 11, 2014

Owner

@jpfuentes2: Many, but not all, of the output functions come in two varieties, Foo and FooOnce. (See metrics.Write and metrics.WriteOnce in writer.go, for example.) The Once versions can be used to do what you're asking. I've simply not gotten around to refactoring all the other output functions to use this pattern.

Owner

rcrowley commented Oct 11, 2014

@jpfuentes2: Many, but not all, of the output functions come in two varieties, Foo and FooOnce. (See metrics.Write and metrics.WriteOnce in writer.go, for example.) The Once versions can be used to do what you're asking. I've simply not gotten around to refactoring all the other output functions to use this pattern.

@pingles

This comment has been minimized.

Show comment
Hide comment
@pingles

pingles Oct 17, 2014

@mihasya i would tend to agree- I added it here because I saw the librato integration :) As you say, much cleaner to not pull in a bunch more dependencies.

I'll see if I can get another repository setup and make all the other suggested changes. I've really appreciated the feedback on the code (new to Go)- any chance I could persuade people to come to the new repo once its up?

pingles commented Oct 17, 2014

@mihasya i would tend to agree- I added it here because I saw the librato integration :) As you say, much cleaner to not pull in a bunch more dependencies.

I'll see if I can get another repository setup and make all the other suggested changes. I've really appreciated the feedback on the code (new to Go)- any chance I could persuade people to come to the new repo once its up?

@pingles

This comment has been minimized.

Show comment
Hide comment
@pingles

pingles Oct 17, 2014

I've made the suggested changes and extracted the code into its own repository: https://github.com/pingles/go-metrics-riemann.

It behaves the same way as before but has a few functions renamed. I'd really appreciate any further feedback.

It feels like it should be possible to extract some kind of Report interface that every reporter can implement and remove the need for each extension to implement the for tick in duration separately?

pingles commented Oct 17, 2014

I've made the suggested changes and extracted the code into its own repository: https://github.com/pingles/go-metrics-riemann.

It behaves the same way as before but has a few functions renamed. I'd really appreciate any further feedback.

It feels like it should be possible to extract some kind of Report interface that every reporter can implement and remove the need for each extension to implement the for tick in duration separately?

@jpfuentes2

This comment has been minimized.

Show comment
Hide comment
@jpfuentes2

jpfuentes2 Oct 17, 2014

It feels like it should be possible to extract some kind of Report interface that every reporter can implement and remove the need for each extension to implement the for tick in duration separately?

That was exactly the kind of thing I was getting at. I'm 👍 there.

jpfuentes2 commented Oct 17, 2014

It feels like it should be possible to extract some kind of Report interface that every reporter can implement and remove the need for each extension to implement the for tick in duration separately?

That was exactly the kind of thing I was getting at. I'm 👍 there.

@jpfuentes2

This comment has been minimized.

Show comment
Hide comment
@jpfuentes2

jpfuentes2 Oct 17, 2014

Ideally, there would be an interface for these emitters/exporters/sinks/what-have-you which makes it easier for authors of extensions to implement their sink.

jpfuentes2 commented Oct 17, 2014

Ideally, there would be an interface for these emitters/exporters/sinks/what-have-you which makes it easier for authors of extensions to implement their sink.

@jpfuentes2

This comment has been minimized.

Show comment
Hide comment
@jpfuentes2

jpfuentes2 Oct 17, 2014

Also, the code looks much better now @pingles, great work :)

jpfuentes2 commented Oct 17, 2014

Also, the code looks much better now @pingles, great work :)

@pingles

This comment has been minimized.

Show comment
Hide comment
@pingles

pingles Oct 17, 2014

@jpfuentes2 thanks!

I'm not particularly sure whether this is a good Go way of doing things, but, off the top of my head... :)

I'm not very familiar with all the other reporters provided inside the project but my guess is you could define a reporter interface as such:

type Reporter interface {
  ReportOnce(metrics.Registry) error
}

ReportOnce would be called to publish all metrics within the registry. Actually, I guess you could go further (given the above ReportOnce would have to iterate across all metrics) and instead define an interface around each metric:

type Reporter interface {
    ReportMetric(string, interface{}) error
}

I guess you could then add a reporter package directly to metrics, a la:

package reporter

func Report(r metrics.Registry, d time.Duration, report *metrics.Reporter) error {
    for _ = range time.Tick(d) {
        r.Each(func(name string, i interface{}) {
            err := report.ReportMetric(name, i)
            if err != nil {
                return err
            }
        }
    }
}

I'm guessing most reporters would need some kind of setup/initialize/connect type procedure also- I guess you could handle the error from Report directly (i.e. re-initialize and then call Report again).

pingles commented Oct 17, 2014

@jpfuentes2 thanks!

I'm not particularly sure whether this is a good Go way of doing things, but, off the top of my head... :)

I'm not very familiar with all the other reporters provided inside the project but my guess is you could define a reporter interface as such:

type Reporter interface {
  ReportOnce(metrics.Registry) error
}

ReportOnce would be called to publish all metrics within the registry. Actually, I guess you could go further (given the above ReportOnce would have to iterate across all metrics) and instead define an interface around each metric:

type Reporter interface {
    ReportMetric(string, interface{}) error
}

I guess you could then add a reporter package directly to metrics, a la:

package reporter

func Report(r metrics.Registry, d time.Duration, report *metrics.Reporter) error {
    for _ = range time.Tick(d) {
        r.Each(func(name string, i interface{}) {
            err := report.ReportMetric(name, i)
            if err != nil {
                return err
            }
        }
    }
}

I'm guessing most reporters would need some kind of setup/initialize/connect type procedure also- I guess you could handle the error from Report directly (i.e. re-initialize and then call Report again).

@pingles

This comment has been minimized.

Show comment
Hide comment
@pingles

pingles Oct 17, 2014

Having just re-read it, given most reporters would then be required to have a large conditional/switch block inside their implementation of ReportMetric maybe its then a case of having separate ReportXxx functions for each metric type:

type Reporter interface {
    ReportCounter(string, metrics.Counter) error
    ReportGauge(string, metrics.Gauge) error
    ReportHistogram(string, metrics.Histogram) error
    ReportMeter(string, metrics.Meter) error
    ReportTimer(string, metrics.Timer) error
}

I guess that removes a bit more of the cruft that each reporter would end up having to implement. Reporters are then responsible for implementing the means for communicating individual metrics, no need to dispatch on type or iterate over durations.

pingles commented Oct 17, 2014

Having just re-read it, given most reporters would then be required to have a large conditional/switch block inside their implementation of ReportMetric maybe its then a case of having separate ReportXxx functions for each metric type:

type Reporter interface {
    ReportCounter(string, metrics.Counter) error
    ReportGauge(string, metrics.Gauge) error
    ReportHistogram(string, metrics.Histogram) error
    ReportMeter(string, metrics.Meter) error
    ReportTimer(string, metrics.Timer) error
}

I guess that removes a bit more of the cruft that each reporter would end up having to implement. Reporters are then responsible for implementing the means for communicating individual metrics, no need to dispatch on type or iterate over durations.

@rcrowley

This comment has been minimized.

Show comment
Hide comment
@rcrowley

rcrowley Nov 8, 2014

Owner

My remaining hesitation to the reporter interface is how to deal with the configuration aspect. Look at the difference between Graphite and syslog, for instance:

func Graphite(r Registry, d time.Duration, prefix string, addr *net.TCPAddr)
type GraphiteConfig struct {
    Addr          *net.TCPAddr  // Network address to connect to
    Registry      Registry      // Registry to be exported
    FlushInterval time.Duration // Flush interval
    DurationUnit  time.Duration // Time conversion unit for durations
    Prefix        string        // Prefix to be prepended to metric names
    Percentiles   []float64     // Percentiles to export from timers and histograms
}
func GraphiteWithConfig(c GraphiteConfig)
func Syslog(r Registry, d time.Duration, w *syslog.Writer)

I can't think of a satisfying way to abstract the timer short of passing closures around and that looks and feels gross to me.

Owner

rcrowley commented Nov 8, 2014

My remaining hesitation to the reporter interface is how to deal with the configuration aspect. Look at the difference between Graphite and syslog, for instance:

func Graphite(r Registry, d time.Duration, prefix string, addr *net.TCPAddr)
type GraphiteConfig struct {
    Addr          *net.TCPAddr  // Network address to connect to
    Registry      Registry      // Registry to be exported
    FlushInterval time.Duration // Flush interval
    DurationUnit  time.Duration // Time conversion unit for durations
    Prefix        string        // Prefix to be prepended to metric names
    Percentiles   []float64     // Percentiles to export from timers and histograms
}
func GraphiteWithConfig(c GraphiteConfig)
func Syslog(r Registry, d time.Duration, w *syslog.Writer)

I can't think of a satisfying way to abstract the timer short of passing closures around and that looks and feels gross to me.

@mihasya

This comment has been minimized.

Show comment
Hide comment
@mihasya

mihasya Jul 4, 2015

Collaborator

While this looks like a lovely PR, I'm going to close it due to the change in policy with regards to client libs.

Please do create a separate repo for this which simply imports go-metrics, and I'll be happy to add it to the README.

Thanks!

Collaborator

mihasya commented Jul 4, 2015

While this looks like a lovely PR, I'm going to close it due to the change in policy with regards to client libs.

Please do create a separate repo for this which simply imports go-metrics, and I'll be happy to add it to the README.

Thanks!

@mihasya mihasya closed this Jul 4, 2015

@mihasya mihasya added the deprecated label Jul 4, 2015

@pingles

This comment has been minimized.

Show comment
Hide comment
@pingles

pingles commented Jul 7, 2015

@mihasya

This comment has been minimized.

Show comment
Hide comment
@mihasya

mihasya Jul 7, 2015

Collaborator

@pingles thank you Paul! Will add to README shortly.

Collaborator

mihasya commented Jul 7, 2015

@pingles thank you Paul! Will add to README shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment