Skip to content

feat: add a span around dialing connections #830

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

Merged
merged 1 commit into from
Apr 16, 2025
Merged

Conversation

steved
Copy link
Contributor

@steved steved commented Apr 16, 2025

If there's some hesitation around creating more disconnected spans, I do think it'd still be useful to only create the span when there's a (rueidis) parent.

I took the span name from go-redis, but perhaps CONNECT might be more appropriate?

I also had some trouble testing that the dial span would be parented to the command appropriately since the pool isn't accessible.

Copy link
Collaborator

@rueian rueian left a comment

Choose a reason for hiding this comment

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

LGTM

@rueian
Copy link
Collaborator

rueian commented Apr 16, 2025

I also had some trouble testing that the dial span would be parented to the command appropriately since the pool isn't accessible.

The span has no chance of being associated with the following command unless we modify the DialCtxFn signature to return the new ctx as well. However, I believe the association isn’t particularly useful overall because even after changing the signature, the span will still not be associated with the following command most of the time since the connection will be reused.

@rueian rueian merged commit d2ee8c7 into redis:main Apr 16, 2025
34 checks passed
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.

2 participants