Skip to content
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

Refactor package metrics #313

Merged
merged 55 commits into from Aug 26, 2016
Merged

Refactor package metrics #313

merged 55 commits into from Aug 26, 2016

Conversation

peterbourgon
Copy link
Member

@peterbourgon peterbourgon commented Jul 8, 2016

This is a complete rewrite of package metrics. Here is the complete new metrics.go file. I'm inlining it because it's pretty and I'm proud of it.

package metrics

// Counter describes a metric that accumulates values monotonically.
// An example of a counter is the number of received HTTP requests.
type Counter interface {
    With(labelValues ...string) Counter
    Add(delta float64)
}

// Gauge describes a metric that takes specific values over time.
// An example of a gauge is the current depth of a job queue.
type Gauge interface {
    With(labelValues ...string) Gauge
    Set(value float64)
}

// Histogram describes a metric that takes repeated observations of the same
// kind of thing, and produces a statistical summary of those observations,
// typically expressed as quantiles or buckets. An example of a histogram is
// HTTP request latencies.
type Histogram interface {
    With(labelValues ...string) Histogram
    Observe(value float64)
}

Major and/or breaking changes:

  • The metric interfaces are reduced to only what is necessary to make observations.
  • metrics.Field is removed. Use ordered string pairs, like package log, instead.
  • All metric values are float64. Thanks, Prometheus, for the inspiration.
  • TimeHistogram is removed. Prefer observing float64 seconds instead.
  • NewMulti{Histogram,Counter,Gauge} are moved to sub-package multi.

Minor changes:

  • PrintHistogram is removed. Users can use generic.Histogram.Print instead.
  • Distribution method is removed. Users can use generic.Histogram.Quantile instead.
  • Metrics systems that have similar emission semantics share similar architectures.
  • Metrics systems that support arbitrary label values share a basic implementation, lv.Space.
  • prometheus.RegisterCallbackGauge is gone. Use standard Prometheus NewGaugeFunc instead.

@peterbourgon peterbourgon changed the title [WIP] Refactor package metrics Refactor package metrics Aug 17, 2016
@peterbourgon
Copy link
Member Author

@jprobinson Would you mind taking a look at the new package provider, and making sure it's still suitable to your needs? I believe it should be straightforward, but you know best :)

@jprobinson
Copy link
Contributor

Sorry for the delay. I like the new look, but need to spend a lil time digging deeper and make sure I don't have any pain points on my side. I'll try to get to it today or tomorrow when I get some time.

@peterbourgon
Copy link
Member Author

Take your time, and thank you!

@jprobinson
Copy link
Contributor

Looks good, @peterbourgon! I've got a PR on Gizmo awaiting the changes.

@peterbourgon
Copy link
Member Author

Great, ship it! Thanks to everyone who helped validate.

@peterbourgon peterbourgon merged commit dd92366 into master Aug 26, 2016
@peterbourgon peterbourgon deleted the new-metrics branch August 26, 2016 10:13
jprobinson added a commit to nytimes/gizmo that referenced this pull request Aug 26, 2016
* metrics updates to match go-kit/kit#313

* changing histograms to use duration.Seconds()
@adrianco
Copy link

adrianco commented Sep 19, 2016

Sorry but you're using the clever stuff in Go that I don't understand well enough so I'm baffled.
I have a metrics.Histogram, which is really an expvar.Histogram and I need to use some definitions from generic.Histogram and it isn't obvious how to do this. i.e. given a metrics.Histogram object h, how do I invoke generics.Histogram.Print on it?
I got it to compile with h.(*generic.Histogram).Print(file) - but it panics at runtime
panic: interface conversion: metrics.Histogram is *expvar.Histogram, not *generic.Histogram

@peterbourgon
Copy link
Member Author

@adrianco You can't access generic.Histogram.Print from an expvar.Histogram. If you want the Print method, you should create and pass around generic.Histogram instead of metrics.Histogram. That's because the metrics.Histogram types have become explicitly write-only.

@adrianco
Copy link

OK, so I've stopped using metrics package completely, and using *generic.Histogram seems to work, but the generic print function doesn't print anything useful.

name: netflixoss.*.*..www00...www.denominator_net
quantiles: [{50 11263} {99 1015807}]
From    To       Count  Prob    Bar
1024    1535     1      0.0500  :#####
2560    3071     1      0.0500  :#####
5632    6143     1      0.0500  :#####
7168    7679     1      0.0500  :#####
7680    8191     2      0.1000  |##########
8192    8703     1      0.0500  |#####
9216    9727     1      0.0500  :#####
10752   11263    3      0.1500  :###############
11776   12287    1      0.0500  :#####
14848   15359    1      0.0500  :#####
15360   15871    1      0.0500  |#####
18432   19455    2      0.1000  :##########
40960   43007    1      0.0500  :#####
45056   47103    1      0.0500  :#####
61440   63487    1      0.0500  :#####
983040  1015807  1      0.0500  :#####

Changed to

Total: 41
2277     ....
3376     ....
3540     ....
3798     ....
3976     ....
3996     ....
4035     ....
4055     ....
4091     ....
4229     ....
4385     ....
4900     ....
5156     ....
5449     ....
5791     ....
5978     ....
6078     ....
6147     ....
6202     ....
6232     ....
6238     ....
6296     ....
6409     ....
6460     ....
6516     ....
6532     ....
6560     ....
6575     ....
6631     ....
6807     ....
7244     ....
7251     ....
7600     ....
8987     ....
9760     ....
14760    ....
28247    ....
66837    ....
73794    ....
101009   ....
193264   ....

@peterbourgon
Copy link
Member Author

Wow, that's not helpful at all! :) Let me dig in a bit.

@adrianco
Copy link

I think the pretty print of histograms was only be in the extvar implementation?

@waylandc
Copy link

Trying to learn to use go-kit and have been following the example at https://gokit.io/examples/stringsvc.html when I came upon problems with the metrics (undefined metrics. Fields). Some googling and I found this refactor. Updated examples would make it easier to learn this stuff. Anyone have link to working examples with the refactored go-kit?

@peterbourgon
Copy link
Member Author

Thanks for the report. I've fixed the website. The code in the repo has always been correct, for the record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants