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

feat(integrations): add support for cluster clients from redis sdk #2394

Merged
merged 13 commits into from
Dec 7, 2023

Conversation

md384
Copy link
Contributor

@md384 md384 commented Sep 27, 2023

This change adds support for cluster clients from the redis sdk (as opposed to the rediscluster library).

This has also been tested in my own app which uses clusters (but not asyncio clusters).

Fixes GH-2523

@md384
Copy link
Contributor Author

md384 commented Oct 15, 2023

I have updated this PR with the db data on the span changes. For redis clusters, I have used the default_node but it might be better to not provide that data at all.

From redis docs - https://redis.readthedocs.io/en/stable/clustering.html#creating-clusters

RedisCluster instance can be directly used to execute Redis commands. When a command is being executed through the cluster instance, the target node(s) will be internally determined. When using a key-based command, the target node will be the node that holds the key’s slot. Cluster management commands and other commands that are not key-based have a parameter called ‘target_nodes’ where you can specify which nodes to execute the command on. In the absence of target_nodes, the command will be executed on the default cluster node. As part of cluster instance initialization, the cluster’s default node is randomly selected from the cluster’s primaries, and will be updated upon reinitialization. Using r.get_default_node(), you can get the cluster’s default node, or you can change it using the ‘set_default_node’ method.

This likely means that it is difficult to get the actual node information and the default node will not always be the one used.

@wgiddens
Copy link

fixes #2523

@sentrivana sentrivana linked an issue Nov 22, 2023 that may be closed by this pull request
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

I took an initial pass at your PR, and I added a few comments and a question. Please address these, and once I hear back, I will review again

sentry_sdk/integrations/redis/__init__.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/redis/__init__.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/redis/__init__.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/redis/__init__.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/redis/__init__.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/redis/asyncio.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/redis/asyncio.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/redis/asyncio.py Outdated Show resolved Hide resolved
tests/integrations/redis/asyncio/test_redis_asyncio.py Outdated Show resolved Hide resolved
@szokeasaurusrex
Copy link
Member

@md384, to get your PR merged as quickly as possible, we would appreciate if you could enable edits from repository maintainers if you are okay with doing so. This way, we can help with any minor changes that need to be made before merging your PR (such as resolving merge conflicts). Thanks!

@md384
Copy link
Contributor Author

md384 commented Nov 28, 2023

@md384, to get your PR merged as quickly as possible, we would appreciate if you could enable edits from repository maintainers if you are okay with doing so. This way, we can help with any minor changes that need to be made before merging your PR (such as resolving merge conflicts). Thanks!

@szokeasaurusrex I have made your suggested changes and enabled access for maintainers. Thanks!

@szokeasaurusrex szokeasaurusrex self-assigned this Nov 28, 2023
@szokeasaurusrex
Copy link
Member

Thank you! I will take a look at your PR again when I have time

Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Hey, thank you for addressing my previous suggestions. I have added a few more comments now; once you address them, I think this PR will be ready to merge!

sentry_sdk/integrations/redis/__init__.py Outdated Show resolved Hide resolved
sentry_sdk/integrations/redis/__init__.py Show resolved Hide resolved
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the previous comments. I just tried to resolve the merge conflict between your branch and master, so your PR would be ready to merge, but I was unsure how to resolve it (please see my inline comment).

sentry_sdk/integrations/redis/__init__.py Show resolved Hide resolved
@szokeasaurusrex
Copy link
Member

Hi @md384, now that I have resolved the merge conflict between your branch and master, I was able to run our CI checks on your code. It looks like your code is failing our linter ("CI / Lint Sources") and our Redis tests. The aws_lambda test is also failing for now, but this is expected, since I need to allow it to run separately; once you have fixed all the other tests, I will run the aws_lambda test.

Please fix the "CI / Lint Sources" and Redis tests, and let me know if you need help with anything!

Comment on lines 34 to 38
# 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"
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

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

I would rewrite the assertions as follows:

Suggested change
# 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"
# on initializing a RedisCluster, a COMMAND call is made - this is not important for the test
# but must be accounted for
assert len(crumbs) in (1, 2)
assert len(crumbs) == 1 || crumbs[0]["message"] == "COMMAND"

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 if statement for the second assertion, it is okay with me to keep that structure, but in either case, please replace event["breadcrumbs"]["values"][0]["message"] with crumbs[0]["message"]

Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

I have a few comments on your most recent changes, but I think this is getting really close

span,
# the AsyncClusterPipeline has always had a `_client` attr but it is private so potentially problematic and mypy
# does not recognize it - see https://github.com/redis/redis-py/blame/v5.0.0/redis/asyncio/cluster.py#L1386
async_redis_cluster_pipeline_instance._client, # type: ignore[attr-defined]
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

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

Comment on lines 34 to 38
# 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"
Copy link
Member

Choose a reason for hiding this comment

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

I would rewrite the assertions as follows:

Suggested change
# 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"
# on initializing a RedisCluster, a COMMAND call is made - this is not important for the test
# but must be accounted for
assert len(crumbs) in (1, 2)
assert len(crumbs) == 1 || crumbs[0]["message"] == "COMMAND"

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 if statement for the second assertion, it is okay with me to keep that structure, but in either case, please replace event["breadcrumbs"]["values"][0]["message"] with crumbs[0]["message"]

if len(crumbs) == 2:
assert event["breadcrumbs"]["values"][0]["message"] == "COMMAND"

crumb = event["breadcrumbs"]["values"][-1]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
crumb = event["breadcrumbs"]["values"][-1]
crumb = crumbs[-1]

Comment on lines 78 to 82
# 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"
Copy link
Member

Choose a reason for hiding this comment

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

My previous comment applies here, as well

Co-authored-by: Matthieu Devlin <matt@zumper.com>
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for the contribution!

@szokeasaurusrex szokeasaurusrex added the Trigger: tests using secrets PR code is safe; run CI label Dec 7, 2023
@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) December 7, 2023 14:14
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Dec 7, 2023
@szokeasaurusrex szokeasaurusrex added the Trigger: tests using secrets PR code is safe; run CI label Dec 7, 2023
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Dec 7, 2023
@szokeasaurusrex
Copy link
Member

szokeasaurusrex commented Dec 7, 2023

It looks like some of the tests are failing; can you please attempt to fix them? Let me know if you have trouble debugging them, and I can help

Never mind, the failing test is known to be flaky, and we have a PR to fix it.

@szokeasaurusrex szokeasaurusrex added the Trigger: tests using secrets PR code is safe; run CI label Dec 7, 2023
@szokeasaurusrex szokeasaurusrex enabled auto-merge (squash) December 7, 2023 15:33
@szokeasaurusrex szokeasaurusrex merged commit 75f89b8 into getsentry:master Dec 7, 2023
470 of 472 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Trigger: tests using secrets PR code is safe; run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support official redis cluster client
3 participants