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

TCP listener for statsd input #2293

Merged
merged 29 commits into from
Aug 8, 2017
Merged

TCP listener for statsd input #2293

merged 29 commits into from
Aug 8, 2017

Conversation

szibis
Copy link
Contributor

@szibis szibis commented Jan 18, 2017

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

This PR adds TCP listener based on tcp_listener input code. We need TCP listener for statsd input in cloud environment's where UDP balancing is not available and for more reliable communication.

This is missing feature that is supported in etsy statsd - https://github.com/etsy/statsd/blob/master/docs/server.md

@sparrc sparrc added this to the 1.3.0 milestone Jan 21, 2017
@danielnelson danielnelson modified the milestones: 1.3.0, 1.4.0 Apr 20, 2017
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.

As you mentioned this code is heavily based on the tcp_listener plugin, but that plugin was deprecated and replaced with socket_listener.

While it might be nicer if this was based on the newer socket_listener code, I think this might be quite a bit more work, as the existing statsd plugin is similar to the old udp_listener.

One concern I have is that I often see intermittent errors with the udp_listener when running unittests. Though looking through the github issues I don't see many reported issues. @phemmer Do you have a sense of if this code might be problematic due to being based on tcp_listener?

CHANGELOG.md Outdated
@@ -96,6 +97,7 @@ be deprecated eventually.
- [#2732](https://github.com/influxdata/telegraf/pull/2732): Use go 1.8.1
- [#2712](https://github.com/influxdata/telegraf/issues/2712): Documentation for rabbitmq input plugin
- [#2141](https://github.com/influxdata/telegraf/pull/2141): Logparser handles newly-created files.
- [#2293](https://github.com/influxdata/telegraf/pull/2293): Add TCP listener for statsd input
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this line, looks a leftover from a rebase.

// Tell the connection why we are closing.
fmt.Fprintf(conn, "Telegraf maximum concurrent TCP connections (%d)"+
" reached, closing.\nYou may want to increase max_tcp_connections in"+
" the Telegraf tcp listener configuration.\n", s.MaxTCPConnections)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming this is not part of the statsd protocol, any chance this will confuse a tcp statsd implementation? Maybe we should just close the connection?

@phemmer
Copy link
Contributor

phemmer commented Jul 17, 2017

@phemmer Do you have a sense of if this code might be problematic due to being based on tcp_listener?

The only part of the tcp_listener plugin which I personally considered bad design was it dropping metrics. It's my stance that on stream protocols, the receiver should not drop any metrics if it cannot process them fast enough. The receiver should instead stop reading which the sender will then notice, and can handle as it sees fit.
Similarly when connection count maxes out, instead of accepting and closing, don't accept. Let the client's TCP timeouts handle it. That way if the count maxes out for just a brief moment, we don't unnecessarily deny a connection.

But neither of these is a major issue, so the code should be fine to use.

@danielnelson
Copy link
Contributor

So, just need to discuss my code comments. In the future we can rebuild stastd input on socket_listener.

@szibis
Copy link
Contributor Author

szibis commented Jul 19, 2017

I can't do the work in next two weeks. After that no problem and I will fix all from comments and stay with tcp_listener (working on our prod from 3 months by now). Is this ok with 1.4.0 release plan ?

@danielnelson
Copy link
Contributor

@szibis Cutoff for 1.4 features is currently around the 16th of August.

@szibis
Copy link
Contributor Author

szibis commented Aug 4, 2017

@danielnelson all comments fixed

CHANGELOG.md Outdated
@@ -50,6 +51,7 @@
- [#3063](https://github.com/influxdata/telegraf/pull/3063): Add tls options to docker input.
- [#2387](https://github.com/influxdata/telegraf/pull/2387): Add histogram aggregator plugin.
- [#3080](https://github.com/influxdata/telegraf/pull/3080): Add zipkin input plugin.
- [#2293](https://github.com/influxdata/telegraf/pull/2293): Add TCP listener for statsd input
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is added twice and there is a merge conflict. Recently I have been adding the changelog updates separately to avoid these merge issues, so you can just remove all changes to this file.

@danielnelson danielnelson merged commit f3435f1 into influxdata:master Aug 8, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants