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

Inject :connection to sql.active_record event instead of using #_id2ref #649

Merged
merged 1 commit into from
Dec 14, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Dec 4, 2018

As an alternative implementation to #647, this adds :connection to Rails sql.active_record event, then uses that object to derive connection configuration for the trace. To accomplish this, it leverages #648 to add a Datadog::Shim::Double into the AbstractAdapter class.

:connection has recently been added to Rails master via rails/rails#34602. The behavior in this PR is for the benefit of Rails versions preceding that change on Rails.

TODO

  • Add :connection to ActiveRecord event
  • Change ActiveRecord integration to use :connection
  • Add specs
  • Compare performance

@delner delner added bug Involves a bug integrations Involves tracing integrations do-not-merge/WIP Not ready for merge labels Dec 4, 2018
@delner delner self-assigned this Dec 4, 2018
@delner
Copy link
Contributor Author

delner commented Dec 10, 2018

I created a pretty simple performance_spec.txt which runs connection.execute('SELECT 42') 100,000 times against an in-memory database and used it to to measure both implementations. Each had comparable performance, with Shim::Double just being a slight bit faster, but both are a bit slower than the control:

Control commit b67871fbde052d36a44e1e7fa2747bce7c2f3c29:

root@d6dc35cafbcb:/app# bundle exec appraisal contrib rspec spec/ddtrace/contrib/active_record/performance_spec.rb
>> BUNDLE_GEMFILE=/app/gemfiles/contrib.gemfile bundle exec rspec spec/ddtrace/contrib/active_record/performance_spec.rb

Randomized with seed 53122

Run time for 100000 iterations: 8.59
.

Finished in 10.45 seconds (files took 0.83915 seconds to load)
1 example, 0 failures

Randomized with seed 53122

Use of Shim::Double and injecting connection arg in this PR commit 9e12665356be00603f153ebe7b8b988a694534a9:

root@d43f6b42218a:/app# bundle exec appraisal contrib rspec spec/ddtrace/contrib/active_record/performance_spec.rb 
>> BUNDLE_GEMFILE=/app/gemfiles/contrib.gemfile bundle exec rspec spec/ddtrace/contrib/active_record/performance_spec.rb

Randomized with seed 39954

Run time for 100000 iterations: 8.93
.

Finished in 10.84 seconds (files took 0.84327 seconds to load)
1 example, 0 failures

Randomized with seed 39954

Using iterative strategy, by searching connections in the connection pool in #647 commit e2613ee11565ebed99e60dc5899f735a75339e57:

root@cb5b6a204da8:/app# bundle exec appraisal contrib rspec spec/ddtrace/contrib/active_record/performance_spec.rb
>> BUNDLE_GEMFILE=/app/gemfiles/contrib.gemfile bundle exec rspec spec/ddtrace/contrib/active_record/performance_spec.rb

Randomized with seed 47628

Run time for 100000 iterations: 8.97
.

Finished in 10.72 seconds (files took 0.86328 seconds to load)
1 example, 0 failures

Randomized with seed 47628

We should probably look at more qualitative differences to determine what's the best strategy, and see if there's something we can do to bring performance closer to the control.

@pawelchcki
Copy link
Contributor

Lookin at the benchmarks I'm s leaning a bit more towards the solution that uses the object proxy/shim. As it seems to me to be a bit more versatile.

@delner
Copy link
Contributor Author

delner commented Dec 12, 2018

Cool. Need to rebase this on the shim PR then after I add specs there, then I'll wrap this one up.

@delner delner force-pushed the feature/add_shim branch 3 times, most recently from 58b4f19 to 134b3a9 Compare December 14, 2018 00:16
@delner delner force-pushed the fix/active_record_avoid_connection_id2ref branch from 9e12665 to 6c3554c Compare December 14, 2018 00:16
@delner delner changed the base branch from feature/add_shim to 0.18-dev December 14, 2018 18:30
@delner delner removed the do-not-merge/WIP Not ready for merge label Dec 14, 2018
@delner delner added this to the 0.18.0 milestone Dec 14, 2018
@delner delner merged commit 8578d60 into 0.18-dev Dec 14, 2018
@delner delner deleted the fix/active_record_avoid_connection_id2ref branch December 14, 2018 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants