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

Fix ActiveRecord adapter name for Rails 7 #3051

Merged
merged 15 commits into from
Aug 22, 2023

Conversation

TonyCTHsu
Copy link
Contributor

@TonyCTHsu TonyCTHsu commented Aug 16, 2023

Closes #2924

What does this PR do?

Fix Utils.adapter_name for version > 7

  1. Replace the condition with respond_to? :db_config, instead of Rails version check
  2. Mock Utils.adapter_name for settings spec to avoid flaky spec
  3. Remove ::ActiveRecord::ConnectionAdapters::ConnectionSpecification::Resolver for connection resolver
  4. Add test with activerecord, 3.x, 4.x, 5.x, 6.0.x, 6.1.x and 7.x

@TonyCTHsu TonyCTHsu added integrations Involves tracing integrations tracing labels Aug 16, 2023
@TonyCTHsu TonyCTHsu added this to the 1.14.0 milestone Aug 16, 2023
@TonyCTHsu TonyCTHsu self-assigned this Aug 16, 2023
@TonyCTHsu TonyCTHsu changed the title Tonycthsu/activerecord util db adapter Fix ActiveRecord adapt name Aug 16, 2023
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/activerecord-util-db-adapter branch from 1de2f86 to 3d8f8a2 Compare August 16, 2023 15:19
@TonyCTHsu TonyCTHsu marked this pull request as ready for review August 16, 2023 16:13
@TonyCTHsu TonyCTHsu requested a review from a team August 16, 2023 16:13
Copy link
Member

@anmarchenko anmarchenko left a comment

Choose a reason for hiding this comment

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

looks good to me, will let someone from apm-ruby team to approve as I still lack context

Appraisals Outdated Show resolved Hide resolved
@TonyCTHsu TonyCTHsu force-pushed the tonycthsu/activerecord-util-db-adapter branch from b2911b7 to 1f86fc9 Compare August 18, 2023 09:32
@codecov-commenter
Copy link

Codecov Report

Merging #3051 (294b0fa) into master (50a2035) will increase coverage by 0.00%.
Report is 40 commits behind head on master.
The diff coverage is 96.15%.

@@           Coverage Diff           @@
##           master    #3051   +/-   ##
=======================================
  Coverage   98.12%   98.12%           
=======================================
  Files        1324     1324           
  Lines       74830    74847   +17     
  Branches     3404     3406    +2     
=======================================
+ Hits        73425    73444   +19     
+ Misses       1405     1403    -2     
Files Changed Coverage Δ
...ng/contrib/active_record/configuration/resolver.rb 94.73% <85.71%> (+0.09%) ⬆️
lib/datadog/tracing/contrib/active_record/utils.rb 88.88% <100.00%> (-2.23%) ⬇️
...ntrib/active_record/configuration/resolver_spec.rb 100.00% <100.00%> (ø)
...ntrib/active_record/configuration/settings_spec.rb 100.00% <100.00%> (ø)
...atadog/tracing/contrib/active_record/utils_spec.rb 100.00% <100.00%> (ø)

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@ekump ekump left a comment

Choose a reason for hiding this comment

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

Not that familiar with this adapter, but LGTM

@@ -587,6 +587,15 @@ elsif ruby_version?('2.3')
appraise 'core-old' do
gem 'dogstatsd-ruby', '~> 4'
end

# Somehow, I failed to install this appraisal group with Ruby 2.4
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I supposed we are going to deprecate old ruby and Rails 3.x in the future, so did not invest too much solving this, so I left a comment instead.

@marcotc marcotc changed the title Fix ActiveRecord adapt name Fix ActiveRecord adapter name for Rails 7 Aug 21, 2023
…ver_spec.rb

Co-authored-by: Marco Costa <marco.costa@datadoghq.com>
@TonyCTHsu TonyCTHsu merged commit 20fa281 into master Aug 22, 2023
162 checks passed
@TonyCTHsu TonyCTHsu deleted the tonycthsu/activerecord-util-db-adapter branch August 22, 2023 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rails version comparison bug in Datadog::Tracing::Contrib::ActiveRecord::Utils.db_config
5 participants