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 instrumentation of custom cache stores #3206

Merged
merged 5 commits into from
Oct 24, 2023
Merged

Fix instrumentation of custom cache stores #3206

merged 5 commits into from
Oct 24, 2023

Conversation

smudge
Copy link
Contributor

@smudge smudge commented Oct 17, 2023

What does this PR do?

This fixes #3199, by avoiding the assumption that config.cache_store will be set to something that exists in the ActiveSupport::Cache:: namespace. By using the generic demodularize, it retains backwards compatibility while also supporting constants such as MyApp::CustomCache or CustomRedisCache.

Motivation:

Much like #3199, I was seeing:

    NoMethodError:
       undefined method `[]' for nil:NilClass

     # ddtrace-1.15.0/lib/datadog/tracing/contrib/active_support/cache/instrumentation.rb:104:in `dd_store_name'
     # ddtrace-1.15.0/lib/datadog/tracing/contrib/active_support/cache/instrumentation.rb:158:in `write'

How to test the change?

Install a cache store that does not exist in the ActiveSupport namespace, such as Rails' new solid_cache. Upon use, the above stack trace will be reproduced.

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@smudge smudge requested a review from a team as a code owner October 17, 2023 23:26
@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Oct 17, 2023
ekump
ekump previously requested changes Oct 20, 2023
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.

Hi @smudge thank you for contributing this bug fix! The fix looks great, but would you be able to add some testing around it? The cache spec can be found here.

@smudge
Copy link
Contributor Author

smudge commented Oct 20, 2023

I've added two new test cases for a cache class in a different namespace and a cache class without a namespace. 👍

@smudge smudge requested a review from ekump October 20, 2023 16:32
@marcotc marcotc dismissed ekump’s stale review October 23, 2023 20:47

Tests added!

@marcotc marcotc added this to the 1.16.0 milestone Oct 23, 2023
@marcotc marcotc merged commit 9bc3c30 into DataDog:master Oct 24, 2023
15 checks passed
@smudge smudge deleted the patch-1 branch October 24, 2023 18:22
ekump pushed a commit that referenced this pull request Oct 25, 2023
ekump added a commit that referenced this pull request Oct 26, 2023
Co-authored-by: Nathan Griffith <[email protected]>
Co-authored-by: Marco Costa <[email protected]>
@marcotc
Copy link
Member

marcotc commented Nov 3, 2023

Hey @smudge, this change has just been released in 1.16.0. Let us know if this addresses your issue!

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.

1.15.0 fails to instrument custom cache stores
3 participants