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

Fix minmax and basicstats aggregators to use all number types #4294

Merged
merged 1 commit into from
Jun 30, 2018

Conversation

adrianlzt
Copy link
Contributor

These aggregators were not working with other numbers type not being
float64 or int64.
Telegraf uses externals libs that could use other types.

Required for all PRs:

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

@adrianlzt
Copy link
Contributor Author

Fix #4293

@danielnelson
Copy link
Contributor

Fields types can now (since 1.6) only be int64, uint64, float64, string, and bool [1]. Let's just add support for uint64 so we don't need to use the reflect package.

There are a variety of unittests that use other units, but the unittests just need to be fixed.

[1]: https://github.com/influxdata/telegraf/blob/master/metric/metric.go#L251

@danielnelson danielnelson added the fix pr to fix corresponding bug label Jun 15, 2018
@danielnelson danielnelson added this to the 1.7.1 milestone Jun 15, 2018
These aggregators were not working with number type uint64
Telegraf uses externals libs that uses this type
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

👍

@danielnelson danielnelson merged commit 85eacf2 into influxdata:master Jun 30, 2018
danielnelson pushed a commit that referenced this pull request Jun 30, 2018
codesmith14 pushed a commit to signalfx/telegraf that referenced this pull request Jul 5, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants