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

Stackdriver sort order #5385

Merged
merged 4 commits into from
Feb 6, 2019
Merged

Stackdriver sort order #5385

merged 4 commits into from
Feb 6, 2019

Conversation

danielnelson
Copy link
Contributor

This change attempts to mitigate #5364 by sorting the batch, but doesn't solve the issue. Remaining issues are documented in the README and some errors are ignored, resulting in some points being dropped, but attempting to keep the output from becoming stuck.

related #5364

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 Feb 6, 2019
@danielnelson danielnelson added this to the 1.10.0 milestone Feb 6, 2019
@danielnelson danielnelson added the area/gcp Google Cloud plugins including cloud_pubsub, cloud_pubsub_push, stackdriver label Feb 6, 2019
@danielnelson
Copy link
Contributor Author

@Legogris This isn't a full fix for the issue, but I think it should help. Do you think you could try it out and see if you run into any major problem? Here are some packages:

@Legogris
Copy link
Contributor

Legogris commented Feb 6, 2019

Awesome @danielnelson, will try it out :)

@Legogris
Copy link
Contributor

Legogris commented Feb 6, 2019

@danielnelson Been running it for a bit, no issues so far. Once this is included in a release we will be able to roll it out to production and see how it behaves there. Really appreciate the quick patch, thanks! We do have an interval of 30s so if we see issues after this we will look into increasing it to 60s or using basicstats.

for i := len(metrics) - 1; i >= 0; i-- {
batch = append(batch, metrics[i])
}
sort.Slice(batch, func(i, j int) bool {
Copy link
Contributor

@glinton glinton Feb 6, 2019

Choose a reason for hiding this comment

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

Why not just sort.Slice(metrics, ..., instead of duplicating it into batch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question, the reason is that outputs are not allowed to modify the batch because if an error occurs the metrics will not be restored to the metric buffer correctly. This is an unfortunate aspect of the output interface, it does save a copy of the buffer but it might be worth the copy to prevent misuse.

@danielnelson danielnelson merged commit 7f54ae1 into master Feb 6, 2019
@danielnelson danielnelson deleted the stackdriver-sort-order branch February 6, 2019 22:17
trevorwhitney pushed a commit to trevorwhitney/telegraf that referenced this pull request Feb 14, 2019
@danielnelson danielnelson modified the milestones: 1.10.0, 1.9.5 Feb 19, 2019
danielnelson added a commit that referenced this pull request Feb 19, 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
Labels
area/gcp Google Cloud plugins including cloud_pubsub, cloud_pubsub_push, stackdriver fix pr to fix corresponding bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants