feat(aggregators): implement histogram aggregator #927

[vmarchaud]

Ported the Go implementation to TS and moved each aggregator to its own file to avoid having one massive file containing all implementations

[codecov-io]

Codecov Report

Merging #930 into master will increase coverage by 0.06%.
The diff coverage is 96.83%.

@@            Coverage Diff             @@
##           master     #930      +/-   ##
==========================================
+ Coverage   94.95%   95.02%   +0.06%     
==========================================
  Files         204      209       +5     
  Lines        8409     8541     +132     
  Branches      763      766       +3     
==========================================
+ Hits         7985     8116     +131     
- Misses        424      425       +1     

Impacted Files	Coverage Δ
...ckages/opentelemetry-metrics/src/export/Batcher.ts	100.00% <ø> (ø)
...try-metrics/src/export/aggregators/measureexact.ts	100.00% <ø> (ø)
packages/opentelemetry-metrics/src/export/types.ts	100.00% <ø> (ø)
packages/opentelemetry-metrics/test/Meter.test.ts	99.34% <ø> (ø)
...emetry-metrics/src/export/ConsoleMetricExporter.ts	62.50% <37.50%> (-9.73%)	⬇️
...metry-metrics/src/export/aggregators/countersum.ts	100.00% <100.00%> (ø)
...emetry-metrics/src/export/aggregators/histogram.ts	100.00% <100.00%> (ø)
...ntelemetry-metrics/src/export/aggregators/index.ts	100.00% <100.00%> (ø)
...lemetry-metrics/src/export/aggregators/observer.ts	100.00% <100.00%> (ø)
...-metrics/test/export/aggregators/histogram.test.ts	100.00% <100.00%> (ø)
... and 5 more

[mayurkale22]

AFAIK only toPoint method is used by exporter to export the data, why do we need to expose sum, count and checkpoint? I think we should expose as minimal as possible to avoid breaking changes later.

[vmarchaud]

As same as above, i've tried to match the go implementation

[vmarchaud]

Edit: From the spec here, we should expose a toCheckpoint rather than toPoint, so thats why i've added this one. However i think we need to rename it in other aggregators, so i'll use toPoint too and we'll make a PR to rename them later.

[dyladan]

I think if it is called toPoint in other aggregators, we should use the same here. If it needs to be renamed, it should be renamed everywhere (including here) in a separate PR. Don't want to be in a state where we have both correct and incorrect naming in the same package.

[mayurkale22]

maybe just reset()?

[mayurkale22]

Just curious to know, who is responsible for calling this method? Does it end user or exporter?

[vmarchaud]

Agree it make more sense. On the call side, the spec clearly define that here, i believe what they call a Integrator is our current Batcher

[mayurkale22]

Looks like reset call is always required to get last updated point, (curious to know) who is responsible for calling this reset on HistogramAggregator? With other aggregators, toPoint() will always gives you current point without making a call to reset. Maybe I am missing something here.

[vmarchaud]

I've answered here: #930 (comment)

[vmarchaud]

Tests failures are related to gts like others PR, i believe we can ignore since it should be fixed when #958 will be merged

[obecny]

lgtm, one issue with naming

[obecny]

why is it plural if it is an object, could this be a bucket ?

[dyladan]

It is describing all of the buckets. For instance if you have three buckets (<10, <20, <30) with measurements of [5, 5, 5, 5, 5, 15, 15, 15, 25], you would have:

hist: Histogram = {
	buckets: {
		boundaries: [10, 20, 30],
		counts: [5, 3, 1],
	},
	sum: 95,
	count: 9
}

[dyladan]

@vmarchaud maybe you can add comments on the interface explaining that the boundaries are upper boundaries, the counts are the number of measurements in the bucket, and so on. I think this will not be clear to people who need to maintain this later if you are not here to walk them through it.

[obecny]

hmm shouldn't we have then

hist: Histogram = {
	buckets: [
		{
			boundary: 10,
			count: 5,
		},
		{
			boundary: 20,
			count: 3,
		},
		{
			boundary: 30,
			count: 1,
		},
	],
	sum: 95,
	count: 9
}

[dyladan]

that would be one different way to do it, but I don't think one way is any more correct than another

[vmarchaud]

I agree that its current form feel non-javascript, it's because i've just ported the same logic from Go to JS. I will try to make it clearer

[vmarchaud]

I've added the following comment to explain why this structure is needed:

/**
   * Buckets are implemented using two different array:
   *  - boundaries contains every boundary (which are upper boundary for each slice)
   *  - counts contains count of event for each slice
   * 
   * Note that we'll always have n+1 (where n is the number of boundaries) slice
   * because we need to count event that are above the highest boundary. This is the
   * reason why it's not implement using array of object, because the last slice
   * dont have any boundary.
   * 
   * Example if we measure the values: [5, 30, 5, 40, 5, 15, 15, 15, 25]
   *  with the boundaries [ 10, 20, 30 ], we will have the following state:
   * 
   * buckets: {
   *	boundaries: [10, 20, 30],
   *	counts: [3, 3, 2, 1],
   * }
   */