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

Introducing hooks #223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Introducing hooks #223

wants to merge 1 commit into from

Conversation

kakkun61
Copy link

This PR introduce hooks. These hooks hook sending requests and receiving responses.

Motivation

I am implementing an instrumentation of OpenTelemetry for Hedis. First I wrapped the Hedis functions to record for tracing, but it cause a lot of type mismatch because the original client and the wrappers have different types of course. So I want to use hooks to avoid this inconvenience.

Changes

  • ConnectInfo has a new field connectHooks
  • Hooks type is introduced

Currently there are five types of hooks:

  • sendRequest
  • sendPubSub
  • callback
  • send
  • receive

sendRequest is called when a command is being sent. sendPubSub is called when a Pub/Sub message is being send. callback is called when a reaction to a Pub/Sub message is being called after receiving its message. send and receive are low-level hooks

Notes

A WIP implementation of the instrumentation of OpenTelemetry for Hedis is herp-inc/hs-opentelemetry#15.

@kakkun61
Copy link
Author

kakkun61 commented Apr 2, 2024

@informatikr How about this? 🤔

connect :: NS.HostName -> CC.PortID -> Maybe Int -> IO Connection
connect hostName portId timeoutOpt = do
connect :: NS.HostName -> CC.PortID -> Maybe Int -> Hooks -> IO Connection
connect hostName portId timeoutOpt hooks = do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since Hooks is a breaking change. Would it make more sense to expose a new function connectWithHooks and the current connect function would internally use defaultHooks so the signature doesn't change.

Copy link
Author

Choose a reason for hiding this comment

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

Sure!

@kakkun61
Copy link
Author

kakkun61 commented Aug 2, 2024

@aravindgopall How about this?

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.

3 participants