search

rcrowley / go-metrics

Go port of Coda Hale's Metrics library
Other
3.43k stars 493 forks source link

Sample interface is not sufficiently exported for use with StandardHistogram #188

Closed kevingessner closed 7 years ago

kevingessner commented 7 years ago

I'd like to implement a new type of Sample, to be used as the sample underlying a Histogram. Rather than forking, I'd prefer to implement it with a struct in my own package. While the Sample interface is fully exported, there is one missing piece that prevents Samples from other packages from being used with the existing StandardHistogram (as returned by NewHistogram).

Any Sample can be used to construct the StandardHistogram, but StandardHistogram.Snapshot explicitly asserts that the sample's Snapshot method returns a SampleSnaphot (rather than a Sample as the Sample interface declares).

Implementations of Sample in other packages cannot construct SampleSnapshots; while the type is exported, its fields are not, and there is no constructor for it. This means that Sample implementations are hamstrung, and can't be used with Histograms.

I see several ways of fixing this:

  1. Export the fields of SampleSnapshot.
  2. Export a NewSampleSnapshot(count int64, values []int64) method for constructing SampleSnapshots
  3. Change StandardHistogram to not require a SampleSnaphot but any Sample for its snapshot (by changing HistogramSnapshot).

Any of those would be acceptable, but I think 3 is the most general: there doesn't seem to be a strong reason to enforce that HistogramSnapshot has a SampleSnaphot rather than any Sample.

mihasya commented 7 years ago

OK, following this through a little bit.. All the protections are in place to ensure the read-only nature of the snapshot returned, which is cool and good. However, your point is very well taken that with the private fields and no constructor, we're defeating the whole point of using interfaces in some of these places. I have asked @rcrowley to rise from his lengthy slumber and weigh in on this just to make sure my understanding above is correct. Assuming it is, I am leaning towards option #2 - exposing a way to create SampleSnapshots while still guaranteeing their contract of being a read-only implementation of Sample.

Thank you for writing up the issue so concisely!

mihasya commented 7 years ago

(and now I'm actually going to do this!)