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

Add ActiveRecord cached tag #291

Merged
merged 3 commits into from
Jan 5, 2018
Merged

Add ActiveRecord cached tag #291

merged 3 commits into from
Jan 5, 2018

Conversation

delner
Copy link
Contributor

@delner delner commented Dec 26, 2017

Add the cached tag to ActiveRecord query spans that are cached.

The old implementation was not correctly tagging ActiveRecord queries that were resolved through the query cache. This pull requests fixes that implementation, adds the same tag for ActiveRecord integrations outside of Rails, and adds test coverage for both scenarios.

Still need to ascertain what milestone this should be applied to.

@delner delner self-assigned this Dec 26, 2017
@delner delner added the do-not-merge/WIP Not ready for merge label Dec 26, 2017
@delner delner removed the do-not-merge/WIP Not ready for merge label Jan 3, 2018
@delner delner added bug Involves a bug enhancement labels Jan 3, 2018
Copy link
Member

@p-lambert p-lambert left a comment

Choose a reason for hiding this comment

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

Great job! 👏
CI is breaking though – waiting for you to fix it

@delner delner force-pushed the add_active_record_cached_tag branch from f80e50a to 7c9ca70 Compare January 3, 2018 22:49
@delner
Copy link
Contributor Author

delner commented Jan 3, 2018

Rubocop strikes again!

@delner delner force-pushed the add_active_record_cached_tag branch from 7c9ca70 to e4529a8 Compare January 3, 2018 23:30
@delner
Copy link
Contributor Author

delner commented Jan 3, 2018

Test failure was some kind of race condition, I think. Writer wasn't dumping its spans before they were asserted upon. Weirdly enough, regardless of how I used try_wait_until, it would sometimes intermittently fail anyways. (e.g., made it attempt 100 times, it would wait for like 10 seconds, then still fail.) Worker threads seemed alive but sleeping. Locked out by mutex or something?

I avoided this altogether by moving my caching ActiveRecord queries into a Sinatra endpoint, and then querying that endpoint to do the AR queries, instead of doing the AR queries directly in my test like I did previously. I honestly don't know why this fixed it, maybe something configuration related, but it seemed to eliminate the race condition for the test, and the need for try_wait_until.

@palazzem palazzem added this to the 0.11.0 milestone Jan 4, 2018
Copy link
Contributor

@palazzem palazzem left a comment

Choose a reason for hiding this comment

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

Good to me, but we need to fix the test suite first.

@delner delner force-pushed the add_active_record_cached_tag branch from e4529a8 to 5935934 Compare January 5, 2018 19:38
@delner
Copy link
Contributor Author

delner commented Jan 5, 2018

Seems to be failing specifically on the old versions of Ruby (1.9.3, 2.0.0, 2.1.10), ActiveRecord (3.2) and Sinatra (1.4.5.)

rvm $MRI_OLD_VERSIONS --verbose do appraisal contrib-old rake test:sinatra

Digging deeper into this.

@delner delner force-pushed the add_active_record_cached_tag branch from 5935934 to 7e1e922 Compare January 5, 2018 21:38
@delner
Copy link
Contributor Author

delner commented Jan 5, 2018

Turns out my assertion didn't match output from different versions of ActiveRecord. Very minor, easy thing to fix, and an oversight on my part.

@delner delner merged commit ea85aa9 into master Jan 5, 2018
@palazzem palazzem deleted the add_active_record_cached_tag branch January 8, 2018 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Involves a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants