Skip to content

Ensure RedisIntegration is disabled, unless redis is installed #2504

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.

Sign up for GitHub

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

Merged
szokeasaurusrex merged 11 commits into from
Nov 18, 2023
Merged

Ensure RedisIntegration is disabled, unless redis is installed #2504

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
069b6e9
Add test to ensure redis integration disabled unless installed
szokeasaurusrex Nov 13, 2023
b039d84
Integrations added to enabled list if actually installed
szokeasaurusrex Nov 13, 2023
0f307bb
Move test to test_basics.py
szokeasaurusrex Nov 13, 2023
b3a81a1
Code review suggestions
szokeasaurusrex Nov 15, 2023
f9ea0cf
Merge branch 'master' into szokeasaurusrex/redis-integration-disabled…
szokeasaurusrex Nov 15, 2023
76841a1
Fixed test failures
szokeasaurusrex Nov 17, 2023
d9bd1fd
Add unit test to check multiple `setup_integrations` calls
szokeasaurusrex Nov 17, 2023
ad47ea3
Merge branch 'master' into szokeasaurusrex/redis-integration-disabled…
szokeasaurusrex Nov 17, 2023
b3dc172
fix type hint for 2.7
szokeasaurusrex Nov 18, 2023
1a63d0d
Added staticmethod
szokeasaurusrex Nov 18, 2023
dc86efe
Move test to `test_basics`
szokeasaurusrex Nov 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions sentry_sdk/integrations/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@


_installer_lock = Lock()

# Set of all integration identifiers we have attempted to install
_processed_integrations = set() # type: Set[str]

# Set of all integration identifiers we have actually installed
_installed_integrations = set() # type: Set[str]


Expand Down Expand Up @@ -121,7 +126,7 @@ def setup_integrations(

for identifier, integration in iteritems(integrations):
with _installer_lock:
if identifier not in _installed_integrations:
if identifier not in _processed_integrations:
logger.debug(
"Setting up previously not enabled integration %s", identifier
)
Expand All @@ -144,8 +149,16 @@ def setup_integrations(
logger.debug(
"Did not enable default integration %s: %s", identifier, e
)
else:
_installed_integrations.add(identifier)
Comment thread
sentrivana marked this conversation as resolved.

_processed_integrations.add(identifier)

_installed_integrations.add(identifier)
integrations = {
identifier: integration
for identifier, integration in iteritems(integrations)
if identifier in _installed_integrations
}

for identifier in integrations:
logger.debug("Enabling integration %s", identifier)
Expand Down
6 changes: 3 additions & 3 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import sentry_sdk
from sentry_sdk._compat import iteritems, reraise, string_types
from sentry_sdk.envelope import Envelope
from sentry_sdk.integrations import _installed_integrations # noqa: F401
from sentry_sdk.integrations import _processed_integrations # noqa: F401
from sentry_sdk.profiler import teardown_profiler
from sentry_sdk.transport import Transport
from sentry_sdk.utils import capture_internal_exceptions
Expand Down Expand Up @@ -187,8 +187,8 @@ def reset_integrations():
with a clean slate to ensure monkeypatching works well,
but this also means some other stuff will be monkeypatched twice.
"""
global _installed_integrations
_installed_integrations.clear()
global _processed_integrations
_processed_integrations.clear()


@pytest.fixture
Expand Down
26 changes: 26 additions & 0 deletions tests/integrations/test_init.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
from sentry_sdk.integrations import Integration, setup_integrations


class NoOpIntegration(Integration):
"""
A simple no-op integration for testing purposes.
"""

identifier = "noop"

def setup_once(): # type: () -> None
pass

def __eq__(self, __value: object) -> bool:
"""
All instances of NoOpIntegration should be considered equal to each other.
"""
return type(__value) == type(self)


def test_multiple_setup_integrations_calls():
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

first_call_return = setup_integrations([NoOpIntegration()], with_defaults=False)
assert first_call_return == {NoOpIntegration.identifier: NoOpIntegration()}

second_call_return = setup_integrations([NoOpIntegration()], with_defaults=False)
assert second_call_return == {NoOpIntegration.identifier: NoOpIntegration()}
24 changes: 22 additions & 2 deletions tests/test_basics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from sentry_sdk._compat import reraise
from sentry_sdk.integrations import _AUTO_ENABLING_INTEGRATIONS
from sentry_sdk.integrations.logging import LoggingIntegration
from sentry_sdk.integrations.redis import RedisIntegration
from sentry_sdk.scope import ( # noqa: F401
add_global_event_processor,
global_event_processors,
Expand All @@ -28,6 +29,18 @@
from sentry_sdk.tracing_utils import has_tracing_enabled


def _redis_installed(): # type: () -> bool
"""
Determines whether Redis is installed.
"""
try:
import redis # noqa: F401
except ImportError:
return False

return True


def test_processors(sentry_init, capture_events):
sentry_init()
events = capture_events()
Expand Down Expand Up @@ -59,8 +72,8 @@ def test_auto_enabling_integrations_catches_import_error(sentry_init, caplog):
sentry_init(auto_enabling_integrations=True, debug=True)

for import_string in _AUTO_ENABLING_INTEGRATIONS:
# Ignore redis in the test case, because it is installed as a
# dependency for running tests, and therefore always enabled.
# Ignore redis in the test case, because it does not raise a DidNotEnable
# exception on import; rather, it raises the exception upon enabling.
if _AUTO_ENABLING_INTEGRATIONS[redis_index] == import_string:
continue

Expand Down Expand Up @@ -686,3 +699,10 @@ def test_functions_to_trace_with_class(sentry_init, capture_events):
assert len(event["spans"]) == 2
assert event["spans"][0]["description"] == "tests.test_basics.WorldGreeter.greet"
assert event["spans"][1]["description"] == "tests.test_basics.WorldGreeter.greet"


@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