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

Floats in statsd percentiles (rebased with master) #5572

Merged

Conversation

Pitxyoki
Copy link
Contributor

@Pitxyoki Pitxyoki commented Mar 12, 2019

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

This addresses #5527. Additionally to the previous PR (#5528), here I'm trying not to do any breaking change:

  • those that used the plugin previously with the percentile array configured with only integer numbers may continue to do so (e.g.: [50, 90, 100]);
  • it is now possible to also configure the same array with only floats (e.g.: [50.0, 90.0, 99.0, 99.9, 100.0]).
  • it is not possible to configure it with a mix of both (e.g.: [50, 90, 99, 99.9, 100]), which I think is reasonable.

This is now targetted at the current master branch.
The already-present Unit Tests were only slightly changed to accept the correct type. No new tests were added.

@Pitxyoki Pitxyoki mentioned this pull request Mar 12, 2019
3 tasks
@Pitxyoki
Copy link
Contributor Author

The circleci tests seem to have failed due to errors in quay.io. Can we re-trigger them?

@Pitxyoki Pitxyoki marked this pull request as ready for review March 13, 2019 11:08
@glinton glinton added area/statsd feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Mar 13, 2019
@Pitxyoki
Copy link
Contributor Author

By the way: what is the process for having this merged into the next minor release for, say, 1.10 and 1.9? I'd like to see this in at least those versions, as those are the current and the likely next versions I'm using.
Should I create different pull requests against the respective branch of each of those releases or do you have some other process for that?

Thanks!

@glinton
Copy link
Contributor

glinton commented Mar 18, 2019

We generally only backport bugfixes, and only to the latest release (1.10 as of now). Features are added to the next minor release (1.11 as of now).

@Pitxyoki
Copy link
Contributor Author

Pitxyoki commented Jun 25, 2019

Hi,
Do you need anything else before this can be merged onto master? I'd like to be sure this will get released on upcoming versions of telegraf.

Thank you.

@danielnelson
Copy link
Contributor

Can you add a unit test that exercises a non-even floating point value, such as the 99.5th percentile?

@Pitxyoki
Copy link
Contributor Author

Can you add a unit test that exercises a non-even floating point value, such as the 99.5th percentile?

@danielnelson: Added, along with other marginal cases.

@danielnelson danielnelson added this to the 1.12.0 milestone Jul 9, 2019
@danielnelson danielnelson merged commit 72c2ac9 into influxdata:master Jul 9, 2019
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/statsd feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants