-
Notifications
You must be signed in to change notification settings - Fork 515
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(redis): Add instrumentation for redis pipeline #1543
Conversation
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.
leaving final review and approval to sdk folks, just some comments
sentry_sdk/integrations/redis.py
Outdated
|
||
def _parse_redis_command(command): | ||
# type: (Any) -> str | ||
return " ".join(map(str, command[0])) |
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.
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.
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.
Done. I factored out the constant.
sentry_sdk/integrations/redis.py
Outdated
|
||
def _parse_redis_command(command): | ||
# type: (Any) -> str | ||
return " ".join(map(str, command[0])) |
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.
Done. I factored out the constant.
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.
thx @jjbayer cool stuff!
just a question / nit about the Strict..
renaming mess
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.
thx, lgtm!
Add automatic instrumentation of redis pipelining for both
redis
andrediscluster
.Note: This does not add instrumentation for
StrictRedisCluster
.