Skip to content

A new Logz.io output plugin#8202

Merged
ssoroka merged 14 commits intoinfluxdata:masterfrom
idohalevi:output-logz
Oct 22, 2020
Merged

A new Logz.io output plugin#8202
ssoroka merged 14 commits intoinfluxdata:masterfrom
idohalevi:output-logz

Conversation

@idohalevi
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@ssoroka ssoroka added this to the 1.16.0 milestone Sep 30, 2020
@idohalevi
Copy link
Copy Markdown
Contributor Author

@ssoroka what do you think?

@ssoroka
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@idohalevi
Copy link
Copy Markdown
Contributor Author

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

@Doron-Bargo
Copy link
Copy Markdown
Contributor

Hi @ssoroka
Any update on this PR?

Copy link
Copy Markdown
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
Copy Markdown
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