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

discuss: exponential histogram addition #31340

Closed
Tracked by #30705
sh0rez opened this issue Feb 20, 2024 · 5 comments
Closed
Tracked by #30705

discuss: exponential histogram addition #31340

sh0rez opened this issue Feb 20, 2024 · 5 comments
Labels

Comments

@sh0rez
Copy link
Contributor

sh0rez commented Feb 20, 2024

Component(s)

processor/deltatocumulative

Describe the issue you're reporting

To convert delta to cumulative, the spec says the following:

Add the current value to the cumulative counter

While that operation is clear for sums ($\text{old}+\text{sample}=\text{next}$), histograms are a more complex data structure:

type Histogram struct {
    count int64
    sum float64
    bucket_counts []int64 // optional
    explicit_bounds []float64 // optional
    min, max float64 // optional
}
  • bucket_counts and explicit_bounds are required when there are buckets at all, which is nearly always the case
  • spec question: can buckets change over time?
  • what do count and sum contain in a delta context?
  • how do we handle min and max?

Let's discuss how this operation should look like and what the edge-cases are
@jpkrohling @gouthamve @RichieSams @djaglowski

@sh0rez sh0rez added the needs triage New item requiring triage label Feb 20, 2024
Copy link
Contributor

Pinging code owners for processor/deltatocumulative: @sh0rez @RichieSams. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@crobert-1
Copy link
Member

This issue still needs discussion, but since it was filed by a code owner I'm removing needs triage.

@RichieSams
Copy link
Contributor

spec question: can buckets change over time?

There's no explicit mention of it that I can see. But my feeling is no. https://opentelemetry.io/docs/specs/otel/metrics/data-model/#histogram

If we see a difference in the bucket bounds, that should be considered an error (similar to us detecting that the start_timestamp is different). And we should call reset()

what do count and sum contain in a delta context?

They would be defined in the same way as in cumulative, except instead of being representative of the entire population, they only represent the subset of the population that was added in this delta metric.

IE, the buckets would only have counts for the new events in the population. And count / sum would be representative of those. Thus to aggregate a delta histogram into a cumulative one, you should be able to just add count, sum, and bucket_counts to the cumulative histogram's representative values.

how do we handle min and max?

I think we can just do:

cumulative.min = math.Min(cumulative.min, delta.min)
cumulative.max = math.Max(cumulative.max, delta.max)

@RichieSams
Copy link
Contributor

RichieSams commented Feb 27, 2024

As far as I can tell, ExponentialHistogram should be fairly similar. Except the bucket boundaries are defined mathematically, instead of explicitly. And it splits bucket_counts into three pieces: positive, zero_count, and negative.

Adding delta.positive / delta.negative with cumulative.positive / cumulative.negative will not be a trivial as in the normal histogram. You might need to modify offset and/or resize the slice. But it should be doable.

@sh0rez sh0rez changed the title discuss: (exponential) histogram addition discuss: exponential histogram addition Mar 28, 2024
@sh0rez
Copy link
Contributor Author

sh0rez commented Mar 28, 2024

implementation for exponential histograms is ongoing: #32030

closing for now, explicit histograms will be done later and separately

@sh0rez sh0rez closed this as completed Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants