Change to use a single socket, to avoid fd sockets leaking…

JustinAiken commented 10 years ago

Because | grep "UDP" | wc -l should never show numbers in the 1000's, or you're going to have a bad time...

bklang commented 10 years ago

Something's not right, I'm getting

/srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/statsd-ruby-1.2.1/lib/statsd.rb:116:in `initialize': wrong number of arguments (3 for 2) (ArgumentError)
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/bundler/gems/adhearsion-stats-f2c217ba9f8f/lib/adhearsion-stats/plugin.rb:20:in `new'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/bundler/gems/adhearsion-stats-f2c217ba9f8f/lib/adhearsion-stats/plugin.rb:20:in `block in <class:Plugin>'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin/initializer.rb:26:in `instance_exec'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin/initializer.rb:26:in `run'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin.rb:168:in `block in init_plugins'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin.rb:167:in `each'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/plugin.rb:167:in `init_plugins'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:241:in `init_plugins'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:63:in `block in start'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:41:in `catch'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:41:in `start'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/initializer.rb:12:in `start'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/cli_commands/ahn_command.rb:104:in `start_app'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/adhearsion-2.5.0/lib/adhearsion/cli_commands/ahn_command.rb:49:in `daemon'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/thor-0.18.1/lib/thor/command.rb:27:in `run'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/thor-0.18.1/lib/thor/invocation.rb:120:in `invoke_command'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/thor-0.18.1/lib/thor.rb:363:in `dispatch'
    from /srv/apps/ringplus_engine/shared/bundle/ruby/1.9.1/gems/thor-0.18.1/lib/thor/base.rb:439:in `start'
    from script/ahn:9:in `<main>'

bklang commented 10 years ago

Ok, figured out that I need to use your branch of statsd-ruby. Before we push this as a gem we'll have to address that.

JustinAiken commented 10 years ago

Aah right, forgot about that.. Wish bundler handled gems from git repos somehow...

I see a few ways to work around this issue:

Just add a note to the readme that you need a fork of statsd-ruby in your app's Gemfile
Release my fork of statsd as it's own gem (statsd-socket-ruby), require that
Just embedded the stasd library directly into this gem...

Any preference or alternative ideas?

bklang commented 10 years ago

As much as I hate forks, it appears that upstream isn't likely to merge your changes.

If the amount of functionality provided is as small as I think it is, I'm inclined to go with Option #3.

benlangfeld commented 10 years ago

I'm not a fan of option 3 due to the duplication and maintenance overhead it creates. Option 1 is not an acceptable solution because it's unnecessarily complicated for the end user. Option 2 would be an acceptable solution, but an upstream merge would be preferable.

Have you been able to contact @raggi to push for your PR to be merged/released? It looks like there's some pending changes unreleased upstream for IPv6 support, and your changes would go nicely in a 1.3 release ;) I think if @raggi can be cajoled into getting that situation resolved, that would be the best for everyone, otherwise go ahead and release a gem from your fork.

JustinAiken commented 10 years ago

For now I've just forked the gem... if my PR ever gets merged, we can update the dependency back to the mainline...

JustinAiken / adhearsion-stats

Change to use a single socket, to avoid fd sockets leaking… #7