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

Ensure RedisIntegration is disabled, unless redis is installed #2504

Merged
17 changes: 15 additions & 2 deletions sentry_sdk/integrations/__init__.py
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)
sentrivana marked this conversation as resolved.
Show resolved Hide 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
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
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
Member Author

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

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

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
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
Loading