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 "multi" methods instrumentation for Rails cache integration #1217

Merged
merged 1 commit into from
Oct 28, 2020

Conversation

michaelkl
Copy link
Contributor

In current implementation only read, write and fetch methods of ActiveSupport::Cache::Store are instrumented. This PR adds the instrumentation of read_multi, write_multi and fetch_multi methods.

image
image

@michaelkl michaelkl requested a review from a team October 22, 2020 19:22
@michaelkl michaelkl marked this pull request as draft October 22, 2020 19:31
@michaelkl michaelkl force-pushed the feat/rails-cache-multi branch 2 times, most recently from 746d934 to b546cc9 Compare October 23, 2020 12:12
@ericmustin
Copy link
Contributor

👋 @michaelkl thanks for the contribution! Generally this looks like a really solid addition to the library. I am a bit backlogged right now but should be able to give this a thoughtful review in the coming days, I want to review how some other libraries and languages handle multi read/write/fetch operations, But i think the approach here makes sense.

Also, for the linting failed tests, bundle exec rake rubocop from the root of the dd-trace-rb aught to surface any linting issues

@michaelkl michaelkl force-pushed the feat/rails-cache-multi branch from b546cc9 to 4516f85 Compare October 23, 2020 12:20
@michaelkl
Copy link
Contributor Author

Thank you @ericmustin ! Glad to help the community!
I tried to follow the same approach already used for existing cache operations.

I will remove the Draft status from the PR when it turns out ready to be reviewed. Take your time, no rush at all!

@michaelkl michaelkl force-pushed the feat/rails-cache-multi branch 2 times, most recently from d8403dd to 84ebc85 Compare October 23, 2020 12:47
@michaelkl michaelkl marked this pull request as ready for review October 23, 2020 13:02
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Great work @michaelkl! I left a few suggestion on my review, but it looks very close to completion. Great job on getting it to pass tests on all (even very old) versions of Rails 👍

@marcotc marcotc added community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations labels Oct 26, 2020
@michaelkl michaelkl force-pushed the feat/rails-cache-multi branch from 84ebc85 to 0a08188 Compare October 27, 2020 06:22
Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Thank you very much, @michaelkl! 🚀

@marcotc marcotc merged commit 220c6fa into DataDog:master Oct 28, 2020
@marcotc marcotc added this to the 0.43.0 milestone Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Was opened by a community member feature Involves a product feature integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants