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 uint support to cratedb output #4117

Merged
merged 3 commits into from
May 8, 2018
Merged

Add uint support to cratedb output #4117

merged 3 commits into from
May 8, 2018

Conversation

danielnelson
Copy link
Contributor

@danielnelson danielnelson commented May 7, 2018

Since 1.6, fields can be int64, uint64, float64, bool, string.

For uint64 values too large to store in CrateDB, we save as the maximum int64 value.

@felixge Can you review?

closes: #4110

Required for all PRs:

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

@danielnelson danielnelson added the fix pr to fix corresponding bug label May 7, 2018
@danielnelson danielnelson added this to the 1.6.2 milestone May 7, 2018
Copy link
Contributor

@felixge felixge left a comment

Choose a reason for hiding this comment

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

LGTM, but see comment.

return strconv.FormatInt(int64(t), 10), nil
} else {
return strconv.FormatInt(MaxInt64, 10), nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

So the idea here is to silently truncate large numbers because CrateDB can't store them? I generally prefer to return an error in these situations, but I think there should be at least a comment explaining the reason for this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is closely based on the behavior of the InfluxDB output but I can definitely leave a comment.

We could log an error here however in most cases the value would probably be oversized on every interval, so we would want to log only once. We can't return an error from Write() because then Telegraf would retry the metric.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@danielnelson danielnelson merged commit b114687 into master May 8, 2018
@danielnelson danielnelson deleted the cratedb-uint branch May 8, 2018 19:10
danielnelson added a commit that referenced this pull request May 8, 2018
jvrahav pushed a commit to jvrahav/telegraf that referenced this pull request May 9, 2018
arkady-emelyanov pushed a commit to arkady-emelyanov/telegraf that referenced this pull request May 18, 2018
maxunt pushed a commit that referenced this pull request Jun 26, 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.

CrateDB output plugin fails with memory input plugin
2 participants