-
Notifications
You must be signed in to change notification settings - Fork 505
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
feat(integrations): add support for cluster clients from redis sdk #2394
Changes from 4 commits
ae5bca6
8734b01
1f125c1
06ab2c2
cd2441a
5c99ddd
3e6e9b6
c868892
78c8795
d5c5a99
3360168
7a77138
eeb534a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,7 +17,7 @@ def monkeypatch_rediscluster_class(reset_integrations): | |||||||||||||||||||
"localhost", 6379 | ||||||||||||||||||||
) | ||||||||||||||||||||
pipeline_cls.execute = lambda *_, **__: None | ||||||||||||||||||||
redis.RedisCluster.execute_command = lambda *_, **__: None | ||||||||||||||||||||
redis.RedisCluster.execute_command = lambda *_, **__: [] | ||||||||||||||||||||
|
||||||||||||||||||||
|
||||||||||||||||||||
def test_rediscluster_breadcrumb(sentry_init, capture_events): | ||||||||||||||||||||
|
@@ -29,7 +29,15 @@ def test_rediscluster_breadcrumb(sentry_init, capture_events): | |||||||||||||||||||
capture_message("hi") | ||||||||||||||||||||
|
||||||||||||||||||||
(event,) = events | ||||||||||||||||||||
(crumb,) = event["breadcrumbs"]["values"] | ||||||||||||||||||||
crumbs = event["breadcrumbs"]["values"] | ||||||||||||||||||||
|
||||||||||||||||||||
# on initializing a RedisCluster, a COMMAND call is made - this is not important for the test | ||||||||||||||||||||
# but must be accounted for | ||||||||||||||||||||
assert len(crumbs) <= 2 | ||||||||||||||||||||
if len(crumbs) == 2: | ||||||||||||||||||||
assert event["breadcrumbs"]["values"][0]["message"] == "COMMAND" | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I mention in the comment, initializing the RedisCluster executes a COMMAND call to the redis server to get available commands. This only happens on the first test run of a parameterized run (more of a problem for test_rediscluster_basic), perhaps there is some caching mechanism somewhere There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rewrite the assertions as follows:
Suggested change
The first assertion should be changed because the test should explicitly verify that at least one breadcrumb was created; the previous assertion would have succeeded with no breadcrumbs and the test would only fail later with an IndexError. Regarding the second assertion, I think my suggested revision conveys more clearly the intent to verify that either there is only one breadcrumb, or if there are two, then the first breadcrumb should be a command. If you prefer the structure with the |
||||||||||||||||||||
|
||||||||||||||||||||
crumb = event["breadcrumbs"]["values"][-1] | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||
|
||||||||||||||||||||
assert crumb == { | ||||||||||||||||||||
"category": "redis", | ||||||||||||||||||||
|
@@ -65,7 +73,15 @@ def test_rediscluster_basic(sentry_init, capture_events, send_default_pii, descr | |||||||||||||||||||
rc.set("bar", 1) | ||||||||||||||||||||
|
||||||||||||||||||||
(event,) = events | ||||||||||||||||||||
(span,) = event["spans"] | ||||||||||||||||||||
spans = event["spans"] | ||||||||||||||||||||
|
||||||||||||||||||||
# on initializing a RedisCluster, a COMMAND call is made - this is not important for the test | ||||||||||||||||||||
# but must be accounted for | ||||||||||||||||||||
assert len(spans) <= 2 | ||||||||||||||||||||
if len(spans) == 2: | ||||||||||||||||||||
assert event["spans"][0]["description"] == "COMMAND" | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My previous comment applies here, as well |
||||||||||||||||||||
|
||||||||||||||||||||
span = event["spans"][-1] | ||||||||||||||||||||
assert span["op"] == "db.redis" | ||||||||||||||||||||
assert span["description"] == description | ||||||||||||||||||||
assert span["data"] == { | ||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we could try to avoid using
_client
here, and instead access what we need through a public API? I would strongly prefer to avoid using an external library's private API, since the private API could change at any time and cause problems in the future.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I don't believe there is a way we can avoid using
_client
. We can use a try/except here to be safe and log when it is unavailable. Is there a standard way to raise sentry internal warnings?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, in that case, we can use
capture_internal_exceptions
here. I also checked that one of your unit tests is verifying this function, so we will get a test failure if_client
ever gets removed