-
Notifications
You must be signed in to change notification settings - Fork 381
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
Use dogstatsd single threaded mode #1576
Conversation
c3cc8a8
to
6ffa554
Compare
I've incorporated changes from #1575 into this PR, so it should have both changes. The only thing missing is gemfile updates, which I'm doing now. |
lib/ddtrace/metrics.rb
Outdated
# IMPORTANT: Once you delete this, you probably want to update the `#ignored_statsd_warning` below where we | ||
# recommend that customers add `gem 'dogstatsd-ruby', '~> 4.0'` to their Gemfile to perhaps state something | ||
# different. | ||
def incompatible_statsd_warning | ||
return if Gem.loaded_specs['dogstatsd-ruby'].version < Gem::Version.new('5.0') | ||
return if dogstatsd_version < Gem::Version.new('5.0') || dogstatsd_version >= Gem::Version.new('5.2') | ||
|
||
INCOMPATIBLE_STATSD_ONLY_ONCE.run do | ||
Datadog.logger.warn( | ||
'This version of `ddtrace` is incompatible with `dogstastd-ruby` version >= 5.0 and can ' \ | ||
'cause unbounded memory usage. Please use `dogstastd-ruby` version < 5.0 instead.' | ||
'`ddtrace` is incompatible with `dogstastd-ruby` versions 5.0.0, 5.0.1, and 5.2.0 and can ' \ | ||
'cause unbounded memory usage. Use `dogstastd-ruby` version >= 5.2 instead.' | ||
) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: In #1575 we had discussed and decided to remove this, and instead just make ddtrace treat 5.0.0, 5.0.1 and 5.1.0 as "incompatible" versions and outright not use them. See the discussion in the PR for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it wasn't a good idea to make it incompatible, but I now agree with metrics being missing for safety. Changes applied.
@@ -30,7 +30,7 @@ gem 'redcarpet', '~> 3.4' if RUBY_PLATFORM != 'java' | |||
gem 'rspec', '~> 3.10' | |||
gem 'rspec-collection_matchers', '~> 1.1' | |||
gem 'rspec_junit_formatter', '>= 0.4.1' | |||
gem 'rspec_n', '~> 1.3' if RUBY_VERSION >= '2.3.0' | |||
gem 'rspec_n', '~> 1.3' if RUBY_VERSION >= '2.4.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latest version of transient dependency cri
is incompatible with Ruby 2.3.
@@ -1,4 +1,4 @@ | |||
require 'bundler/stub_specification' | |||
require 'bundler' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprise fix for Bundler::StubSpecification
access 🤷
Codecov Report
@@ Coverage Diff @@
## master #1576 +/- ##
==========================================
+ Coverage 98.24% 98.25% +0.01%
==========================================
Files 886 886
Lines 42687 42685 -2
==========================================
+ Hits 41939 41942 +3
+ Misses 748 743 -5
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
This PR uses the new
single_thread
mode fordogstatsd-ruby
clients instantiated byddtrace
.This allows us to have the benefits of batch metric submission of
dogstatsd-ruby
, without memory issues we've seen with the5.x
releases.This PR adds a new test combination for the affected
5.x
versionsdogstatsd-ruby
, to ensure we are informing users that they should use a different version of the library in order to have a more stable environment.