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

Development/test environment in Rails #130

Open
ngan opened this issue Feb 8, 2020 · 5 comments
Open

Development/test environment in Rails #130

ngan opened this issue Feb 8, 2020 · 5 comments

Comments

@ngan
Copy link

ngan commented Feb 8, 2020

We use this gem at my company and we recently started see "not enough file descriptor"-like errors in our Rails app. We found at that this gem makes makes a new connections to port localhost:8125 for every single call to StatsD. On MacOS, the limit is 256.

Anyways, my question is this: What's best practice for using this gem in development/test environment?

I see two possibilities:

  1. Don't use this gem in development/test environment. Perhaps use a different "mocked" client library, but then keeping the API consistent is a burden.
  2. In development/test, mock out calls so that it doesn't actually do anything. Other StatsD client libraries (eg https://github.com/Shopify/statsd-instrument#configuration) are RAILS_ENV aware and basically only log the call in development/test. No network calls made.

I think 2 is ideal, but this library does not support this. Nor does it provide an easy way of accomplishing this in a DIY manner. The closest I found were tips from this issue: #28. But looking at the way your tests are doing it, instance_variable_set seems very brittle to me.

Are there plans to make this client library more development-environment friendly? I can find some time to make this library RAILS_ENV-aware as well if you guys would like.

@kbogtob
Copy link
Contributor

kbogtob commented Feb 10, 2020

Hi @ngan!

Thanks for your feedback! l will investigate about the new connection at each call, I'll see if I can reproduce. Do you have any additional clues about that?

About the development mode, it's in the work backlog but I can't tell you when it will be shipped. The code is currently being refactored and tests are being rewritten.

Cheers!

@ngan
Copy link
Author

ngan commented Feb 10, 2020

Do you have any additional clues about that?

Unfortunately, I don't. We've patched it to not send requests and moved on from the issue. But basically every time we make a $redis.increment(...) or whatever call, we'd see a new connection with to port 8125 using lsof -i:8125.

@arielvalentin
Copy link

I am not sure if this is the best issue to comment on but I would be keen on being able to use a strategy similar to the FakeUDPSocket that this project uses in unit tests.

https://github.com/DataDog/dogstatsd-ruby/blob/master/spec/statsd_spec.rb#L4

Perhaps the constructor could take an additional value for socket_type that would use the fake implementation instead e.g. socket_type: :test

@tatey
Copy link

tatey commented May 11, 2020

@arielvalentin Me too! I've written two pieces of middleware based on dogstatsd. I used the FakeUDPSocket strategy to write the unit tests.

If there's appetite I'd be happy to write a patch for dogstatsd did would let you do something like this:

require "dogstatsd/test_helper"

def test_the_thing
  socket = Datadog::FakeUDPSocket.new
  statsd = Datadog::Statsd.new(socket: socket)
  statsd.increment("foo.bar")
  assert_equal ["foo.bar:1|c"], socket.recv
end

Does that seem like a reasonable approach? Otherwise if we were to take it one step further we could do something like what @arielvalentin is proposing:

def test_the_thing
  statsd = Datadog::Statsd.new(socket_path: :test)
  statsd.increment("foo.bar")
  assert_equal ["foo.bar:1|c"], statsd.test_socket.recv
end

@kbogtob Would you accept a patch that did something like this?

@jamesjefferies
Copy link

How about setting up the DataDog config so that in the test environment, you have a simple configuration item:

  Datadog.configure do |c|
    c.tracer.enabled = false
  end

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

No branches or pull requests

5 participants