-
Notifications
You must be signed in to change notification settings - Fork 375
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
Fix Ruby VM statistics broken and causing errors on Ruby 3.2 #2600
Conversation
`Tracer#initialize` only takes keyword arguments so I did the simple fix rather than the full `ruby2_keywords` thing.
context 'with Ruby < 3', if: RUBY_VERSION < '3.0.0' do | ||
context 'on Ruby 2.x' do | ||
before { skip('Test only runs on Ruby 2.x') unless RUBY_VERSION.start_with?('2.') } |
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.
Why replace if: RUBY_VERSION < '3.0.0'
with the explicit skip?
I like the explicit skip when some test is not running, because it shows up in the rspec output that the testcase was skipped.
Otherwise, it's easy to do a change, run RSpec, it says all tests passed and you just broke some other Ruby version which is being skipped. Yes CI would catch it, but I still favor the more explicit style.
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 like this approach as well.
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.
Approved, but left a few open questions that may be resolved later.
@@ -57,15 +57,27 @@ def flush | |||
|
|||
try_flush do | |||
if Core::Environment::VMCache.available? | |||
gauge( | |||
# Only on Ruby < 3.2 | |||
gauge_if_not_nil( |
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.
Pondering about this nil
check + comments.
- WDYT about an explicit version check? (
if
orcase RUBY_VERSION when ...
). That would make the code self-explanatory, and materialise theelse
case. - WDYT about a feature check? something like
if Core::Utils::Features::Ruby.vmcache_global_constant_state?
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.
WDYT about an explicit version check? (if or case RUBY_VERSION when ...). That would make the code self-explanatory, and materialise the else case.
I chose not to do the explicit version check so that in the future we wouldn't get bitten by the same issue: let's say Ruby 3.5 removes some of the new metrics, and again we would break if we checked the Ruby version and not the underlying data.
It's true that our comments could become inaccurate in that situation, but since our tests validate the expected results, we would have an indication that there's something we need to fix -- while still not breaking released dd-trace-rb versions.
WDYT about a feature check? something like if Core::Utils::Features::Ruby.vmcache_global_constant_state?
A feature check per metric would mean more code + more tests to replace the current nil
check. I wouldn't oppose such a change, but I'm not particularly convinced it's worth the extra complexity either.
allow(Datadog::Tracing::Tracer).to receive(:new).and_wrap_original do |method, **args, &block| | ||
instance = method.call(**args, &block) |
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.
Minor: make it explicit that this is keyword arguments now.
allow(Datadog::Tracing::Tracer).to receive(:new).and_wrap_original do |method, **args, &block| | |
instance = method.call(**args, &block) | |
allow(Datadog::Tracing::Tracer).to receive(:new).and_wrap_original do |method, **kwargs, &block| | |
instance = method.call(**kwargs, &block) |
Also, wondering what happened to *args
: is is not needed anymore?
My spider-sense is tingling here WRT the argument changes that happened between 2.6, 2.7, 3.0+3.1, and 3.2.
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 that this is a specific simpler fix for the Tracer
class only -- if this was something on some outside gem we'd need to sprinkle our friend ruby2_keywords
instead ;)
What happened here is that on dd-trace-rb 0.x, this used to be the classic options = {}
:
https://github.com/DataDog/dd-trace-rb/blob/v0.54.2/lib/ddtrace/tracer.rb#L76
...but on 1.0 it was moved to use real keyword arguments:
https://github.com/DataDog/dd-trace-rb/blob/v1.0.0/lib/datadog/tracing/tracer.rb#L50
Up to 3.2, the forwarding we were doing here still worked, but it stopped working in 3.2. And for all the others, this change works because the class gets keyword arguments.
What does this PR do?:
This PR fixes #2534 by:
a. Making sure we don't try to report non-existing statistics on Ruby 3.2
b. Adding reporting of the new Ruby 3.2 statistics
It also includes a few cleanups to get us in better shape to support Ruby 3.2.
Motivation:
dd-trace-rb should definitely not get in the way of our awesome customers using Ruby 3.2.
Additional Notes
The newly-reported metrics will not show up in the Datadog UX yet, I'm preparing a separate PR to the Datadog UX and default Ruby Runtime Metrics dashboard to show them.
How to test the change?:
We are not yet running Ruby 3.2 in CI (I'm preparing a follow-up PR for this), so you'll need to run the test suite locally to validate this.
Fixes #2534