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

Connect hook #1803

Closed
wants to merge 3 commits into from
Closed

Connect hook #1803

wants to merge 3 commits into from

Conversation

pfreixes
Copy link
Contributor

@pfreixes pfreixes commented Jun 28, 2021

Addresses #1786

Added a new PoolHook interface allowing the user to add two new hooks that would be called before and after the creation of a connection, allowing the user - among other stuff - to measure the connection latency.

User still uses the AddHook interface but providing an extended interface which supports the two methods required by the pool.

An example can be found here https://github.com/go-redis/redis/pull/1803/files#diff-ee4b7901a404913de455c4b45c583a3d0bf51e6c3abc06c93c2a1fa6c3eac235R16

What is missing

Does not support the clone behaviour used by functions like Client.WithTimeout which for the client hooks it creates a list by using a shallow copy, decoupling hooks from the former and the new cloned client.

Current proposal does not support this, since the pool is shared by all of the cloned clients.

@vmihailenco
Copy link
Collaborator

Have you considered using OpenTelemetry Metrics to measure the data you need? We already have some metrics and probably should not wait for a stable release to add more.

I understand that the more hooks the better, but this PR adds considerable amount of code and complexity.

@pfreixes
Copy link
Contributor Author

We are already measuring op latency and errors by using the current hooks, which I guess is what OpenTelemetry would provide. I would like to make and step forward and also measure create connection latency.

If this PR does not not move forward, we will do that by overriding the dialer.

@vmihailenco
Copy link
Collaborator

👍 I just meant that OpenTelemetry should work (at least in theory) with Prometheus/Grafana/whatever. So may be adding it to your stack could help.

@pfreixes
Copy link
Contributor Author

Ill close the PR, if eventually other users are interested we can reborn this.

@pfreixes pfreixes closed this Jun 30, 2021
@vmihailenco
Copy link
Collaborator

Sounds good - thanks 👍

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.

None yet

2 participants