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 carbon2 serializer #5345

Merged
merged 8 commits into from
Jan 26, 2019

Conversation

frankreno
Copy link
Contributor

Required for all PRs:

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

This PR addresses issue #5344 by adding a Carbon2 serializer. Carbon2 is a metrics format supported by multiple applications and vendors. Implementing it as a serializer would allow it to integrate with the existing output plugin ecosystem as well as integrate with any additional vendors who want to add support for the Carbon2 format.

@frankreno
Copy link
Contributor Author

Ok, looks like we need to support Go 1.9 which does not have StringBuilders. I'll refactor code to account for that. Any idea on when Telegraf will drop support for 1.9?

@frankreno
Copy link
Contributor Author

Ok great, tests are looking good. Please let me know if you need any changes or have any questions, would love to get this into the earliest release we can.

@sgmeyer
Copy link

sgmeyer commented Jan 25, 2019

🥇 I tested this out with numerous inputs and it worked quite well. I would love to see this merged as our project would like to use this w/ SumoLogic instead of the graphite serializer. This makes Telegraf/Sumo configuration trivial w/ HTTP output.

Copy link
Contributor

@danielnelson danielnelson 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, I just have a few ideas for improving the performance, mostly artifacts from remove StringBuilder. Not sure when we will drop support for Go 1.9, the main sticking point is that Go 1.10 and later don't work on RHEL5, we will probably move if/when it begins to become a larger burden.

plugins/serializers/carbon2/carbon2.go Outdated Show resolved Hide resolved
plugins/serializers/carbon2/carbon2.go Outdated Show resolved Hide resolved
plugins/serializers/carbon2/carbon2.go Outdated Show resolved Hide resolved
@frankreno
Copy link
Contributor Author

@danielnelson thanks much for the review and suggestions. Just pushed the updates, will monitor tests but local tests passed.

@danielnelson danielnelson added this to the 1.10.0 milestone Jan 26, 2019
@danielnelson danielnelson merged commit a153053 into influxdata:master Jan 26, 2019
@danielnelson
Copy link
Contributor

Nice work, thanks!

@frankreno frankreno deleted the add-carbon2-serializer branch January 26, 2019 02:09
trevorwhitney pushed a commit to trevorwhitney/telegraf that referenced this pull request Feb 14, 2019
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants