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.

Sign up for GitHub

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

Jump to bottom

[StatsD receiver] Add timing/histogram for statsD receiver as OTLP summary #3261

Merged
merged 1 commit into from
May 3, 2021
Merged

[StatsD receiver] Add timing/histogram for statsD receiver as OTLP summary #3261

merged 1 commit into from
May 3, 2021

Conversation

gavindoudou
Copy link
Contributor

@gavindoudou gavindoudou commented Apr 27, 2021 •
edited
Loading

Description:

  • This PR is for timing/histogram support using OTLP summary with aggregation. StatsD receiver receives ms or h StatsD type and transfers to OTLP summary. The receiver will aggregate in each interval and transfer the float array(with all values) to a OTLP summary metric.
    For example: the receiver receives in one interval:
    statsdTestMetric2:2|ms|#mykey:myvalue
    statsdTestMetric2:2|ms|#mykey:myvalue
    statsdTestMetric2:5|ms|#mykey:myvalue
    statsdTestMetric2:6|ms|#mykey:myvalue
    It will generate a OTLP summary metric with percentile 0, 10, 50, 90, 95, 100.
  • Remove match in previous PR's config since we don't have regex support yet. Discussed with Josh and Tigran about it. They both agreed to remove math.

Link to tracking Issue:
#2566
previous PR: #2973
Testing:
Added unit tests
Documentation:

@gavindoudou gavindoudou requested a review from jmacd as a code owner April 27, 2021 19:20
@gavindoudou gavindoudou requested a review from a team April 27, 2021 19:20
@gavindoudou gavindoudou changed the title Add timing/histogram for statsD receiver as OTLP summary [StatsD receiver] Add timing/histogram for statsD receiver as OTLP summary Apr 28, 2021
@bogdandrutu
Copy link
Member

@jmacd can you review this?

jmacd
jmacd approved these changes Apr 30, 2021
View reviewed changes
Copy link
Contributor

@jmacd jmacd 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.

receiver/statsdreceiver/README.md
`"observer_type"` specifies OTLP data type to convert to. The only supported target data type currently is `"gauge"`, which does not perform any aggregation.
Support for `"summary"` data type is planned to be added in the future.
`"observer_type"` specifies OTLP data type to convert to. We support `"gauge"` and `"summary"`. For `"gauge"`, it does not perform any aggregation.
For `"summary`, the statsD receiver will aggregate to one OTLP summary metric for one metric description(the same metric name with the same tags). It will send percentile 0, 10, 50, 90, 95, 100 to the downstream.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the quantiles configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The quantiles are fixed.

@jmacd
Copy link
Contributor

jmacd commented Apr 30, 2021

Question: The use of montanaflynn/stats is appropriate in the sense that it will give a correct result. I was wondering if it will be useful, though, since these quantile estimates will be not very useful on a small number of points.

In a Prometheus setting, the Summary metric uses a smoothed calculation that covers 5-10minutes of state, so that even when a small number of points arrive in each interval, the numbers are relatively table.

I wonder about a TODO or an option to use the Prometheus Summary logic directly, though it will mean a bit of investigation to see if it can be extracted from prometheus/client_golang?

Thanks!

@gavindoudou
Copy link
Contributor Author

Do you mean we could have different aggregation intervals for summary and others?

@jmacd
Copy link
Contributor

jmacd commented Apr 30, 2021

The Prometheus Summary uses https://github.com/beorn7/perks/tree/master/quantile.

I wasn't suggesting to change the interval, but suppose there is a 10 second interval. Using a smoothed summary means that every 10 seconds we'll output the summary values averaged over the last 5 minutes or so. I don't want you to change this PR, and happy if it remains a TODO, but I think that will be more useful to the users.

@gavindoudou
Copy link
Contributor Author

gavindoudou commented Apr 30, 2021 •
edited
Loading

Oh, I see. Added a TODO in README. Thanks!

@bogdandrutu bogdandrutu merged commit af6248b into open-telemetry:main May 3, 2021
@gavindoudou gavindoudou deleted the statsDSummaryTimer branch May 3, 2021 22:35
alexperez52 referenced this pull request in open-o11y/opentelemetry-collector-contrib Aug 18, 2021
@sincejune
Remove unnecessary functions in PrometheusRemoteWriteExporter (#3261)
db28270 
* remove unnecessary functions

* rename functions

* update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmacd jmacd jmacd approved these changes

@bogdandrutu bogdandrutu bogdandrutu approved these changes

Assignees

@jpkrohling jpkrohling

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gavindoudou @bogdandrutu @jmacd @jpkrohling