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

Instrument RedisCluster clients #1177

Merged
merged 10 commits into from
Jul 7, 2022

Conversation

sungwonh
Copy link
Contributor

@sungwonh sungwonh commented Jul 2, 2022

Description

This PR adds instrumentation to RedisCluster clients in the Redis library. (redis.cluster.RedisCluster, redis.asyncio.cluster.RedisCluster)

Closes #1167

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Functional tests are added to test_redis_functional.py

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@sungwonh sungwonh requested a review from a team July 2, 2022 12:47
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 2, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: sungwonh / name: Sungwon Han (acff3f4)

@sungwonh
Copy link
Contributor Author

sungwonh commented Jul 6, 2022

@srikanthccv
Thank you for your feedback. According to your feedback, this PR is updated.

Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

LGTM, Just one more suggestion.

Comment on lines 175 to 186
cmds = [
_format_command_args(c.args if hasattr(c, "args") else c[0])
for c in command_stack
]
resource = "\n".join(cmds)

span_name = " ".join([args[0] for args, _ in instance.command_stack])
span_name = " ".join(
[
(c.args[0] if hasattr(c, "args") else c[0][0])
for c in command_stack
]
)
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 also suggest to safe guard this code for potential IndexErrors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to handle IndexError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to handle AttributeError and IndexError in one try block.

@lzchen
Copy link
Contributor

lzchen commented Jul 6, 2022

@sungwonh

From the constributing guidelines, would you like to be added as a component owner for the redis instrumentation?

@@ -149,7 +152,8 @@ def _traced_execute_command(func, instance, args, kwargs):
) as span:
if span.is_recording():
span.set_attribute(SpanAttributes.DB_STATEMENT, query)
_set_connection_attributes(span, instance)
if hasattr(instance, "connection_pool"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add the hasattr check in the _set_connection_attributes function?

Copy link
Contributor Author

@sungwonh sungwonh Jul 6, 2022

Choose a reason for hiding this comment

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

moved hasattr check to the _set_connection_attributes function.

@sungwonh
Copy link
Contributor Author

sungwonh commented Jul 6, 2022

From the constributing guidelines, would you like to be added as a component owner for the redis instrumentation?

@lzchen
Yes, I would. 👍

@ocelotl ocelotl merged commit 8823655 into open-telemetry:main Jul 7, 2022
@sungwonh sungwonh deleted the feature/instrument-rediscluster branch July 8, 2022 10:11
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.

[Redis] RedisCluster support
4 participants