-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Moved to using the inbuilt serializer. #1942
Conversation
@@ -169,23 +166,30 @@ func (k *KinesisOutput) Write(metrics []telegraf.Metric) error { | |||
|
|||
r := []*kinesis.PutRecordsRequestEntry{} | |||
|
|||
for _, p := range metrics { | |||
for _, metric := range metrics { | |||
atomic.AddUint32(&sz, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it was this way before this PR but is there a need for this to be atomic? Same for line 188.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call I will up date this.
And thanks for the feedback!
return m, nil | ||
} | ||
func (k *KinesisOutput) SetSerializer(serializer serializers.Serializer) { | ||
k.serializer = serializer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this break existing users that are using a custom format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how the custom format came to be but it seemed to me to be a placeholder, it actually contains duplicate data which makes it harder to parse..
That said I would not introduce this without calling out the change in format, I myself had to trim off the swap,map[host:rufus]
(see sample below) to parse it using the influx format.
So in summary that this change goes from influx format with prefix containing duplicate data, to just influx format so I think it is justified.
swap,map[host:rufus],swap,host=rufus free=104853504i,in=0i,out=0i,total=104853504i,used=0i,used_percent=0 1476434910000000000
mem,map[host:rufus],mem,host=rufus active=143302656i,available=898482176i,available_percent=92.57988410421336,buffered=41435136i,cached=149790720i,free=729427968i,inactive=65368064i,total=970493952i,used=72011776i,used_percent=7.420115895786644 1476434910000000000
Thanks for this @wolfeidau, but you'll need to rebase before we can merge this. You'll need to change the code that handles the serialized metrics, because we've changed this interface to return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs rebase and slight refactor of handling of "serializer" return value
6952acb
to
81d4dea
Compare
Thanks @sparrc I have rebased and adjusted for the metric type change. Cheers! |
81d4dea
to
2fbca33
Compare
* Moved to using the inbuilt serializer. * Remove Atomic variable as it is not required. * Adjusted metric type in line with latest changes.
* Moved to using the inbuilt serializer. * Remove Atomic variable as it is not required. * Adjusted metric type in line with latest changes.
To enable more flexibilty around the output format to kinesis I have refactored it to use the inbuilt serialisation package.
The test was removed as the only thing it tested was a custom formatting function which has been removed.
Required for all PRs: