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

Remove exact aggregator for histogram instruments #2348

Merged
merged 10 commits into from
Nov 15, 2021

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Nov 5, 2021

This aggregator causes confusing behavior, especially when paired with the OTLP exporter behavior. This aggregator and the corresponding AggregatorSelector (selector/simple.NewWithExactDistribution()) gave users a misleading route from histogram instruments to gauge measurements.

Fixes #2346.

@codecov
Copy link

codecov bot commented Nov 8, 2021

Codecov Report

Merging #2348 (e14815a) into main (5f5280b) will decrease coverage by 0.2%.
The diff coverage is 95.2%.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2348     +/-   ##
=======================================
- Coverage   73.7%   73.5%   -0.3%     
=======================================
  Files        175     174      -1     
  Lines      12439   12298    -141     
=======================================
- Hits        9179    9043    -136     
+ Misses      3023    3015      -8     
- Partials     237     240      +3     
Impacted Files Coverage Δ
...otlp/otlpmetric/internal/metrictransform/metric.go 77.9% <ø> (-1.2%) ⬇️
exporters/prometheus/prometheus.go 65.2% <ø> (ø)
sdk/export/metric/aggregation/aggregation.go 0.0% <ø> (ø)
sdk/metric/processor/processortest/test.go 84.6% <ø> (-1.0%) ⬇️
sdk/metric/selector/simple/simple.go 100.0% <ø> (ø)
bridge/opencensus/aggregation.go 95.7% <93.3%> (-4.3%) ⬇️
bridge/opencensus/exporter.go 100.0% <100.0%> (ø)
sdk/metric/aggregator/aggregatortest/test.go 83.5% <0.0%> (-1.9%) ⬇️
...s/otlp/otlptrace/internal/connection/connection.go 14.6% <0.0%> (-1.6%) ⬇️
... and 3 more

Copy link
Contributor

@evantorrie evantorrie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'm not an expert (nor even conversant) on OpenCensus, so it may be useful to have someone who understands OpenCensus more to take a look at the changes there.

bridge/opencensus/aggregation.go Outdated Show resolved Hide resolved
bridge/opencensus/aggregation.go Outdated Show resolved Hide resolved
bridge/opencensus/exporter.go Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor

dashpole commented Nov 9, 2021

Just to confirm my understanding, since the bridge is now a LastValue aggregation, the opencensus bridge might skip points if the opencensus ReportingInterval is less than the opentelemetry CollectPeriod? If my understanding is correct, can we document that the CollectPeriod should be at least the ReportingInterval?

@jsuereth
Copy link

jsuereth commented Nov 9, 2021

@dashpole ... the opencensus bridge might skip points if the opencensus ReportingInterval is less than the opentelemetry CollectPeriod?

OpenCensus reports cumulative metrics, so this is probably ok if you're actually exporting a gauge or sum, because the last seens value is the "latest cumulative" so dropping is "free" there.

As an FYI -> I'm trying to migrate the OpenCensus bridge in Java to read directly against the OpenCensus storage rather than use an interval-based reading hook. Not sure that applies to Go, but look here for the details.

+1 to removing Exact distribution @jmacd !! This was very confusing name.

Co-authored-by: ET <evantorrie@users.noreply.github.com>
@jmacd
Copy link
Contributor Author

jmacd commented Nov 9, 2021

@dashpole @jsuereth If I understand correctly, there is no loss of data. The OTLP exporter supported the Exact aggregator by making an array of gauges. I've replaced that with a bunch of normal Gauges that will be joined into the same array of gauges here:

m.GetGauge().DataPoints = append(m.GetGauge().DataPoints, res.Metric.GetGauge().DataPoints...)

(See also #2119)

@jmacd
Copy link
Contributor Author

jmacd commented Nov 9, 2021

I should add that #2119 is calling to eliminate the append() that joins multiple point values into an array, because it seems like an unnecessary map-building exercise for the ordinary code path, which exports one point per attribute set, thus we expect len(DataPoints) == 1 in the common case.

#2119 also calls to make the reader interface 3-levels from 2-levels, so the export pattern would be like:

for each library {
  for each instrument {
    for each attribute set {
      write(point)
    }
  }
}

You could make the argument for a 4-level pattern w/ an additional for each point { ... } where we support collection interval != export interval. This may be necessary to finish View support in this SDK.

@dashpole
Copy link
Contributor

Thanks for the explanation!

CHANGELOG.md Outdated Show resolved Hide resolved
bridge/opencensus/aggregation.go Outdated Show resolved Hide resolved
@bogdandrutu
Copy link
Member

/easycla

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@MrAlias MrAlias merged commit 6483b5c into open-telemetry:main Nov 15, 2021
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.

Remove the sdk/metric/aggregator/exact aggregator
7 participants