metrics: add support for PCP by suyash · Pull Request #363 · go-kit/kit

suyash · 2016-09-14T20:00:29Z

Based on the discussion in #354

This is basically the work in my repo except for the readme changes

I have not squashed the commits as GitHub allows squashing all commits into one at merge step, however if it is important, I can squash all changes and submit just one commit

peterbourgon

Thanks very much! In general the comments need a lot of cleanup, and you should remove the panics. Also, the histogram needs a way to Observe in units of seconds (see comment on that method). But this is a good start! I think it won't be a lot of work to resolve.

peterbourgon · 2016-09-15T16:13:25Z

+import (
+	"github.com/go-kit/kit/metrics"
+	"github.com/performancecopilot/speed"
+)


Please group imports according to the canonical style, that is,

import ( // stdlib // third-party, not Go kit // Go kit )

peterbourgon · 2016-09-15T16:14:00Z

+	"github.com/performancecopilot/speed"
+)
+
+// Reporter encapsulates a speed client


Thanks for the method comments! Please use complete sentences, including proper punctuation and grammar, i.e.

// Reporter encapsulates a speed client.

peterbourgon · 2016-09-15T16:14:26Z

+func NewReporter(appname string) *Reporter {
+	c, err := speed.NewPCPClient(appname)
+	if err != nil {
+		panic(err)


Is that necessary? Can it return (*Reporter, error) instead?

peterbourgon · 2016-09-15T16:14:53Z

+	c *speed.PCPClient
+}
+
+// NewReporter creates a new reporter instance


Please use proper punctuation, as above.

Please describe a bit the meaning of the appname parameter, and the behavior if there is some error or misconfiguration.

peterbourgon · 2016-09-15T16:15:24Z

+	return &Reporter{c}
+}
+
+// Start starts reporting currently registered metrics to the backend


Please use proper punctuation (further comments of this type elided).

What does "backend" mean?

using 'PCP installation' instead of backend

peterbourgon · 2016-09-15T16:18:46Z

+// but that will mean this will need a mutex, to be safe
+func (c *Counter) Add(delta float64) { c.c.Inc(int64(delta)) }
+
+////////////////////////////////////////////////////////////////////////////////////////


Remove this separator.

peterbourgon · 2016-09-15T16:19:10Z

+func (r *Reporter) NewGauge(name string, desc ...string) *Gauge {
+	g, err := speed.NewPCPGauge(0, name, desc...)
+	if err != nil {
+		panic(err)


Same comment re: panic vs. returning an error.

peterbourgon · 2016-09-15T16:19:35Z

+
+// NewHistogram creates a new Histogram
+// minimum observeable value is 0
+// maximum observeable value is 3600000000


Shouldn't these values be parameters?

peterbourgon · 2016-09-15T16:19:46Z

+func (r *Reporter) NewHistogram(name string, desc ...string) *Histogram {
+	h, err := speed.NewPCPHistogram(name, 0, 3600000000, 5, desc...)
+	if err != nil {
+		panic(err)


Similar comment re: panic vs. returning an error.

peterbourgon · 2016-09-15T16:20:54Z

+//
+// this converts float64 value to int64, as the Histogram in speed
+// is backed using codahale/hdrhistogram, which only observes int64 values
+func (h *Histogram) Observe(value float64) { h.h.MustRecord(int64(value)) }


Simply converting to an int64 is going to be problematic, because it is idiomatic to Observe in a histogram in units of seconds, which typically are well less than 1. How should this be supported, do you think?

The histogram implementation isn't native and simply wraps a PCP metric around codahale/hdrhistogram, which only records int64 (https://godoc.org/github.com/codahale/hdrhistogram#Histogram.RecordValue), which in turn is based on a java version, which itself records only long (http://hdrhistogram.github.io/HdrHistogram/JavaDoc/org/HdrHistogram/AbstractHistogram.html#recordValue(long)).

Raw PCP metrics allow setting a unit (https://github.com/performancecopilot/speed#singletonmetric), and I have shorted it to OneUnit at most places as it represents Count unit and most of the times we just want to count stuff. But I can provide a units parameter in histogram, similar to Timer (https://github.com/performancecopilot/speed#timer) so users can customize the unit to Milliseconds, Microseconds, Nanoseconds and report int64.

The other option is to replace the underlying histogram with another implementation. I like hdrhistogram for its ideas on precision and reduced memory footprint, but am open to ideas.

so the normal tests were taking 1.6s but tests with -race enabled were taking 29s on my local machine and 38s on travis, so did a small profile

from what I can see, the iterator next method on hdrhistogram seems slow, or maybe I am doing something wrong

I'm fine with keeping HDRHistogram. I like the Unit idea, it should be settable at metric construction time. Testing with -race will always be much slower than otherwise; I wouldn't worry about profiling it.

suyash · 2016-10-06T03:22:36Z

@peterbourgon sorry for the late update on this

have added an additional unit parameter to the histogram construction, both here and to speed (https://godoc.org/github.com/performancecopilot/speed#NewPCPHistogram)
as you said I have added some more comments to all custom metric construction methods for speed
however, the test time for histogram is still a concern to me, mainly I want to understand why it would take that long with race enabled. The tests for speed only increase by ~1 second with race enabled. Still investigating that, as well as trying to study other histogram implementations, will update this thread once I finalize something.

peterbourgon · 2016-10-10T07:41:46Z

+// NewReporter creates a new Reporter instance.
+// The first parameter is the application name and is used to create the speed client.
+// Hence it should be a valid speed parameter name and should not contain spaces or the path separator
+// for your operating system.


Please wrap this and all comment blocks at 80 columns.

peterbourgon · 2016-10-10T07:42:54Z

+
+// Add implements Counter.
+// speed Counters only take int64.
+// speed Gauges can take float64 and Add(float64) is implemented by metrics.Gauge.


Please fix the capitalization in this and all comment blocks, and clarify the behavior.

// Add implements Counter. Speed counters only take int64, so // the delta is converted to an int64 before observation.

peterbourgon · 2016-10-10T07:43:18Z

+
+// NewHistogram creates a new Histogram.
+// minimum observeable value is 0.
+// maximum observeable value is 3600000000.


Please fix capitalization here, and wrap the comment block at 80 columns.

peterbourgon · 2016-10-10T07:45:01Z

+// Observe observes a value.
+//
+// this converts float64 value to int64, as the Histogram in speed
+// is backed using codahale/hdrhistogram, which only observes int64 values.


Please mention something about the value being interpreted as the unit passed to the constructor.

peterbourgon · 2016-10-10T07:45:32Z

Please don't waste time finding out why tests take longer to run with the race detector enabled. The race detector adds a lot of overhead by design.

peterbourgon · 2016-10-10T07:45:58Z

With those final cleanups I've requested, I'm happy to merge. Thanks for taking the time!

suyash · 2016-10-13T14:26:43Z

@peterbourgon as requested I have updated the comment blocks to be a maximum of 80 columns in length.

Moreover, I have generated the godoc for the package on my fork, which can be seen at (https://godoc.org/github.com/suyash/kit/metrics/pcp).

I am using the package name in small characters everywhere (speed instead of Speed), because of the package name.

W.e.f the histogram test time issue, the final call is yours, even if I change something in speed, my goal will be to keep things 100% backwards compatible. Right now whenever I can find the time, I am trying to understand the underlying working of hdrhistogram. In case there arises a need for a change in anything in this package, I can always open another PR.

Thanks for being patient with me, with the code reviews. The generated godoc LGTM, please let me know if you need anything else. I am using atom to check the line width, and except for line 106, everything looks to be inside.

peterbourgon · 2016-10-17T08:02:25Z

Thanks for all of the hard work. I'll make a few other minor cleanups post-merge but this is great. 💯

suyash · 2016-10-17T12:50:14Z

Thanks ✨, you forgot to close the issue, will close it manually.

metrics: add support for PCP

suyash added 5 commits September 15, 2016 01:24

metrics: pcp: initial version

ffa4231

metrics: pcp: add tests

17cdd15

metrics: pcp: add reporting

962040e

metrics: pcp: add support for descriptions

b9af4aa

metrics: pcp: remove global state

8620ade

peterbourgon requested changes Sep 15, 2016

View reviewed changes

suyash added 3 commits September 20, 2016 22:25

metrics/pcp: cleanup the comments

0471dc5

metrics/pcp: fix tests

4e88882

metrics/pcp: add unit to histogram

56f89df

peterbourgon reviewed Oct 10, 2016

View reviewed changes

suyash added 2 commits October 13, 2016 19:26

metrics/pcp: add more comments and fix comment block widths

fe3c57e

metrics/pcp: small capitalization fixes

b89055b

peterbourgon approved these changes Oct 17, 2016

View reviewed changes

peterbourgon merged commit e049844 into go-kit:master Oct 17, 2016

suyash deleted the pcp-go-kit-pr branch October 17, 2016 12:49

suyash mentioned this pull request Oct 17, 2016

metrics: add support for PCP #354

Closed

jamesgist pushed a commit to jamesgist/kit that referenced this pull request Nov 1, 2024

Merge pull request go-kit#363 from suyash/pcp-go-kit-pr

72e127f

metrics: add support for PCP

Uh oh!

Conversation

suyash commented Sep 14, 2016

Uh oh!

peterbourgon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suyash commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbourgon Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterbourgon commented Oct 10, 2016

Uh oh!

peterbourgon commented Oct 10, 2016

Uh oh!

suyash commented Oct 13, 2016

Uh oh!

peterbourgon commented Oct 17, 2016

Uh oh!

suyash commented Oct 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

suyash commented Oct 6, 2016 •

edited

Loading

peterbourgon Oct 10, 2016 •

edited

Loading