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

Add functions for extracting count and sum metrics from Histograms #22853

Closed
swiatekm opened this issue May 29, 2023 · 9 comments
Closed

Add functions for extracting count and sum metrics from Histograms #22853

swiatekm opened this issue May 29, 2023 · 9 comments
Labels
enhancement New feature or request processor/transform Transform processor

Comments

@swiatekm
Copy link
Contributor

swiatekm commented May 29, 2023

Component(s)

processor/transform

Is your feature request related to a problem? Please describe.

I have a histogram, but I'm only interested in the sum and count values. I'd like to extract them as individual metrics and drop the rest of the histogram.

Describe the solution you'd like

I propose that we add convert_histogram_count_val_to_sum and convert_histogram_count_val_to_count functions and deprecate the existing convert_summry_count_val_to_sum and convert_summary_count_val_to_count functions. The new functions would work for Summaries, Histograms and ExponentialHistograms.

My reasoning is that this operation - extracting the count or sum value - is logically identical for all of these metric types, and the implementation is very similar as well. I used histogram in the function name because this obviously covers Histograms and ExponentialHistograms, while Summaries are effectively a legacy metric type, so they should be given less weight here.

In terms of implementation, the new functions would be very similar to the ones for summary, with some additional logic for copying between the different DataPoint types.

Describe alternatives you've considered

I also considered deleting all the buckets from the histogram, but this is both incompatible with the spec and currently impossible using OTTL. Extracting the metrics via an OTTL function feels like the most idiomatic solution.

Additional context

Prometheus handles this use case correctly if we tell it to only scrape the sum and count parts of a histogram, so this would improve our interoperability.

@swiatekm swiatekm added enhancement New feature or request needs triage New item requiring triage labels May 29, 2023
@github-actions github-actions bot added the processor/transform Transform processor label May 29, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@TylerHelmuth
Copy link
Member

I'm not the best at metrics, but Histograms are essentially Summaries with extra meaning right? The logic for converting a Summary to a Sum/Count would be the same for a Histogram?

@swiatekm
Copy link
Contributor Author

swiatekm commented May 31, 2023

Yeah, I believe the code would be almost exactly the same, and it should be easy to have a generic function covering all of these, if that's what we want. Fundamentally what we're doing is just extracting a part of the histogram/summary datapoint into a new metric.

I think this might even be doable with just the metrics transform processor and OTTL as-is, but there's no way to iterate over the datapoints.

@TylerHelmuth
Copy link
Member

I would definitely support merging functions if we can do it in a meaningful way, even if it'd be a breaking change. Based on the existing transformprocessor functions, can you propose how we could handle the use cases for Summary/Histogram/ExpoHistogram (if applicable) in a *to_sum and *to_count functions?

@swiatekm
Copy link
Contributor Author

Sure, I'll verify that a generic function will work the way I expect and then submit a concrete proposal for the change.

@swiatekm
Copy link
Contributor Author

swiatekm commented Jul 18, 2023

I've updated the description with the proposal for the change, and also linked a draft PR adding the sum conversion function. @TylerHelmuth can you have a look and let me know if that PR looks about right?

@swiatekm
Copy link
Contributor Author

swiatekm commented Jul 20, 2023

See also the count implementation: swiatekm#2449. It uses some machinery from the sum PR, so I'd like to get that merged first. I can also submit both at once, but that doesn't look easier to review.

TylerHelmuth pushed a commit that referenced this issue Aug 3, 2023
**Description:**
Added a function for extracting the sum from a Histogram,
ExponentialHistogram or Summary as an individual metric. The added
function also works for Summaries, so we can later deprecate the
Summary-specific `summary_sum_val_to_sum` function.

**Link to tracking Issue:** #22853 

**Testing:**
Added unit tests both for the function itself and OTTL statements
involving it.

**Documentation:**
Added a section in the README.

---------

Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com>
@swiatekm
Copy link
Contributor Author

swiatekm commented Aug 4, 2023

In the course of discussing #24368, we ended up with the following:

  • The function is named extract_sum_metric
  • It doesn't take an aggregation temporality argument, but instead copies it from the source metric. For Summaries, it assumes cumulative, but we plan to add an optional argument to change this.
  • When passed a metric of the wrong type, it throws an error instead of ignoring it

TylerHelmuth pushed a commit that referenced this issue Aug 8, 2023
**Description:**
Added a function for extracting the count from a Histogram,
ExponentialHistogram or Summary as an individual metric. The added
function also works for Summaries, so we can later deprecate the
Summary-specific `summary_count_val_to_sum` function.

This is very similar to #24368, which added a function for extracting
the sum in the same fashion. The only significant difference is that
count values are required by the spec. See that PR for implementation
discussion.

**Link to tracking Issue:** #22853 

**Testing:**
Added unit tests both for the function itself and OTTL statements
involving it.

**Documentation:**
Added a section in the README.
@swiatekm
Copy link
Contributor Author

swiatekm commented Aug 9, 2023

Resolved in #24368 and #24897

@swiatekm swiatekm closed this as completed Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request processor/transform Transform processor
Projects
None yet
Development

No branches or pull requests

3 participants