Skip to content

Ensure RedisIntegration is disabled, unless redis is installed#2504

Merged
szokeasaurusrex merged 11 commits intomasterfrom
szokeasaurusrex/redis-integration-disabled-by-default
Nov 18, 2023
Merged

Ensure RedisIntegration is disabled, unless redis is installed#2504
szokeasaurusrex merged 11 commits intomasterfrom
szokeasaurusrex/redis-integration-disabled-by-default

Conversation

@szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Nov 13, 2023

This PR fixes a bug where the RedisIntegration was showing up in the list of enabled integrations even when redis was not installed, and the integration should have been disabled.

Fixes #1734

szokeasaurusrex
szokeasaurusrex commented Nov 13, 2023
View reviewed changes
tests/test_api.py Outdated
Comment on lines +128 to +132
@pytest.mark.skipif(REDIS_INSTALLED, reason="skipping because redis is installed")
def test_redis_disabled_when_not_installed(sentry_init):
sentry_init()

assert Hub.current.get_integration(RedisIntegration) is None
Copy link
Member Author

@szokeasaurusrex szokeasaurusrex Nov 13, 2023

Choose a reason for hiding this comment

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

I was unsure about in which file this test should be placed. Is it ok to keep here, or should it go somewhere else?

Copy link
Contributor

@sentrivana sentrivana Nov 13, 2023

Choose a reason for hiding this comment

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

Maybe test_basics.py would be better? It already has some tests for integration enabling.

test_api.py is more for testing the global API (configure_scope, start_transaction, etc.; the stuff we surface via

sentry-python/sentry_sdk/__init__.py

Lines 13 to 43 in cb7299a

__all__ = [ # noqa
"Hub",
"Scope",
"Client",
"Transport",
"HttpTransport",
"init",
"integrations",
# From sentry_sdk.api
"capture_event",
"capture_message",
"capture_exception",
"add_breadcrumb",
"configure_scope",
"push_scope",
"flush",
"last_event_id",
"start_span",
"start_transaction",
"set_tag",
"set_context",
"set_extra",
"set_user",
"set_level",
"set_measurement",
"get_current_span",
"get_traceparent",
"get_baggage",
"continue_trace",
"trace",
]
).

@szokeasaurusrex szokeasaurusrex marked this pull request as ready for review November 13, 2023 14:50
@sentrivana
Copy link
Contributor

sentrivana commented Nov 13, 2023 •
edited
Loading

@szokeasaurusrex If I understood the issue correctly, the underlying problem is that in the Redis integration we raise DidNotEnable in setup_once rather than on module level. If that's the case, would moving the check

sentry-python/sentry_sdk/integrations/redis/__init__.py

Lines 275 to 278 in cb7299a

try:
from redis import StrictRedis, client
except ImportError:
raise DidNotEnable("Redis client not installed")
to the top level imports, just like we do in other integrations, solve the issue?

@szokeasaurusrex
Copy link
Member Author

szokeasaurusrex commented Nov 13, 2023

@sentrivana Apparently it is also a more general issue. Integrations which conditionally raise DidNotEnable in the setup_once function are also affected, according to the original issue. From the issue:

However, other automatically enabled integrations that raise DidNotEnable in setup_once for other reasons also encounter this issue, e.g. aiohttp with v3.3

So, raising DidNotEnable when importing the RedisIntegration would probably fix the issue specifically for Redis, but we would still probably need this change to fix the more general problem.

@sentrivana
Copy link
Contributor

sentrivana commented Nov 13, 2023

@szokeasaurusrex Gotcha, I see now. We still raise DidNotEnable in setup_once in virtually all integrations when we succeed in importing the package, but it's somehow incompatible (unsupported version, failure to parse the version altogether, etc.).

I'll give this a proper review tomorrow 👍🏻

@szokeasaurusrex
Copy link
Member Author

szokeasaurusrex commented Nov 13, 2023

We still raise DidNotEnable in setup_once in virtually all integrations when we succeed in importing the package, but it's somehow incompatible (unsupported version, failure to parse the version altogether, etc.).

Yes, and this is precisely the reason why the change here is needed

sentrivana
sentrivana reviewed Nov 14, 2023
View reviewed changes
sentrivana
sentrivana approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

szokeasaurusrex
szokeasaurusrex commented Nov 17, 2023
View reviewed changes
tests/integrations/test_init.py Outdated
return type(__value) == type(self)


def test_multiple_setup_integrations_calls():
Copy link
Member Author

@szokeasaurusrex szokeasaurusrex Nov 17, 2023

Choose a reason for hiding this comment

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

@sentrivana Do you think this new file is an appropriate place for this test?

Copy link
Contributor

@sentrivana sentrivana Nov 17, 2023

Choose a reason for hiding this comment

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

Why not just put it in tests/test_basics.py since we already have some integration tests in there?

Or maybe we could rename this new file from test_init.py to something more generic like test_common.py and also move other integrations-related tests from test_basics.py here.

Copy link
Member Author

@szokeasaurusrex szokeasaurusrex Nov 18, 2023

Choose a reason for hiding this comment

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

Okay, I'll put it in test_basics.py for now. Maybe in the future we could split out into a separate file, but I think moving everything over would be out of scope for this issue

@szokeasaurusrex
Copy link
Member Author

szokeasaurusrex commented Nov 17, 2023

@sentrivana I added a test to check that multiple setup_integrations calls work as expected. I already validated that the test fails on b039d84. Would appreciate if you could quickly review the new test.

@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) November 18, 2023 01:18
@szokeasaurusrex szokeasaurusrex merged commit b3ccf96 into master Nov 18, 2023
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/redis-integration-disabled-by-default branch November 18, 2023 01:24
@Harmon758
Copy link
Contributor

Harmon758 commented Nov 18, 2023

Thanks for the fix! and for ensuring it covers everything I detailed in the issue 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@antonpirker antonpirker Awaiting requested review from antonpirker

@sentrivana sentrivana Awaiting requested review from sentrivana

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Redis integration considered enabled even when Redis client not installed

3 participants

@szokeasaurusrex @sentrivana @Harmon758