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

A new Logz.io output plugin #8202

Merged
merged 14 commits into from
Oct 22, 2020
Merged

A new Logz.io output plugin #8202

merged 14 commits into from
Oct 22, 2020

Conversation

idohalevi
Copy link
Contributor

Required for all PRs:

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

@idohalevi idohalevi mentioned this pull request Sep 30, 2020
3 tasks
@idohalevi
Copy link
Contributor Author

@p-zak @ssoroka ready for the review :)

@ssoroka ssoroka added this to the 1.16.0 milestone Sep 30, 2020
plugins/outputs/logzio/README.md Outdated Show resolved Hide resolved
plugins/outputs/logzio/logzio.go Outdated Show resolved Hide resolved
plugins/outputs/logzio/logzio.go Outdated Show resolved Hide resolved
plugins/outputs/logzio/logzio_test.go Show resolved Hide resolved
@idohalevi
Copy link
Contributor Author

@ssoroka what do you think?

@ssoroka
Copy link
Contributor

ssoroka commented Oct 7, 2020

It's an improvement, but you still have other tests connecting to the logz.io service. I was kind of expecting a net/http/httptest httptest.NewServer (example) that would check the request and mock the response.

@idohalevi
Copy link
Contributor Author

@ssoroka I think I'm missing something, which test is connecting to the Logz.io server? The connect function just go to initializeSender but not actually connecting to Logz.io's server

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

Ok, so I took a closer look at everything and I have two main concerns.

  1. There's no test of the Write() function. If anything was broken in there, you probably wouldn't know, but at least the code is simple right now.
  2. I looked closer at the Send() function from https://github.com/logzio/logzio-go/blob/master/logziosender.go, and I noticed that it has its own internal buffer, which competes with Telegraf's buffer, so essentially you've double buffered the output at twice the cost of memory for it. Not only this, but when the second logzio buffer is full, you drop metrics, meaning users can't control the buffer size from the config like other plugins anymore. The fix for this is to remove the use of logzio-go and just use a simple http connection to write the messages out. 90% of that package is buffering anyway, and you can just copy out the one http write function that you need.

Let me know what you think and if you have any questions.

plugins/outputs/logzio/logzio.go Outdated Show resolved Hide resolved
@idohalevi
Copy link
Contributor Author

@ssoroka I've changed the implementation to use a default HTTP client

@Doron-Bargo
Copy link
Contributor

Hi @ssoroka
Any update on this PR?

Copy link
Contributor

@ssoroka ssoroka left a comment

Choose a reason for hiding this comment

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

I think you're good now. I kind of don't like that the connect function is used as an initializer (there's an Init() error function signature you can use for that), but you'd still need an empty Connect() function if you did that, so, I'll leave it up to you. Let me know when you're ready to merge.

@sjwang90 sjwang90 modified the milestones: 1.16.0, 1.15.4 Oct 19, 2020
@sjwang90 sjwang90 modified the milestones: 1.16.0, Planned Oct 21, 2020
@idohalevi
Copy link
Contributor Author

@ssoroka we're ready to merge :)

@ssoroka ssoroka merged commit 9b23a04 into influxdata:master Oct 22, 2020
@idohalevi idohalevi deleted the output-logz branch October 22, 2020 20:06
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
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.

4 participants