Skip to content

feat(redis): Add instrumentation for redis pipeline#1543

Merged
sl0thentr0py merged 11 commits into
masterfrom
feat/redis-pipeline
Aug 11, 2022
Merged

feat(redis): Add instrumentation for redis pipeline#1543
sl0thentr0py merged 11 commits into
masterfrom
feat/redis-pipeline

Conversation

@jjbayer

@jjbayer jjbayer commented Aug 5, 2022

Copy link
Copy Markdown
Member

Add automatic instrumentation of redis pipelining for both redis and rediscluster.

Note: This does not add instrumentation for StrictRedisCluster.

@jjbayer
jjbayer requested review from beezz and untitaker August 5, 2022 11:40
Comment thread sentry_sdk/integrations/redis.py

@untitaker untitaker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

leaving final review and approval to sdk folks, just some comments

Comment thread sentry_sdk/integrations/redis.py Outdated
Comment thread sentry_sdk/integrations/redis.py Outdated

def _parse_redis_command(command):
# type: (Any) -> str
return " ".join(map(str, command[0]))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i would align this with the code at the bottom of this file. there we use repr instead of str and no list comprehension/slicing. Perhaps you find a way to factor this out.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done. I factored out the constant.

Comment thread sentry_sdk/integrations/redis.py Outdated
Comment thread sentry_sdk/integrations/redis.py
Comment thread sentry_sdk/integrations/redis.py Outdated

def _parse_redis_command(command):
# type: (Any) -> str
return " ".join(map(str, command[0]))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done. I factored out the constant.

Comment thread sentry_sdk/integrations/redis.py Outdated
Comment thread sentry_sdk/integrations/redis.py
Comment thread sentry_sdk/integrations/redis.py
@jjbayer
jjbayer marked this pull request as ready for review August 8, 2022 09:59
@jjbayer
jjbayer requested a review from a team August 8, 2022 09:59
@sl0thentr0py sl0thentr0py self-assigned this Aug 8, 2022

@sl0thentr0py sl0thentr0py left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thx @jjbayer cool stuff!
just a question / nit about the Strict.. renaming mess

Comment thread sentry_sdk/integrations/redis.py Outdated
Comment thread tests/integrations/rediscluster/test_rediscluster.py Outdated
@jjbayer
jjbayer requested a review from sl0thentr0py August 11, 2022 11:09

@sl0thentr0py sl0thentr0py left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

thx, lgtm!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants