-
Notifications
You must be signed in to change notification settings - Fork 19
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
Generalize ResourceLogOps.log to support arbitrary callbacks #237
base: master
Are you sure you want to change the base?
Conversation
@@ -146,21 +147,34 @@ object CatsHelper { | |||
} | |||
} | |||
|
|||
implicit final class ResourceLogOps[F[_], A](val self: Resource[F, A]) extends AnyVal { | |||
def log(name: String)(implicit F: Sync[F], log: Log[F], md: MeasureDuration[F]): Resource[F, A] = Resource { | |||
implicit final class ResourceObservabilityOps[F[_], A](val self: Resource[F, A]) extends AnyVal { |
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.
this is not backward compatible change :(
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 think this is just how diff is displayed. I didn't rename ResourceLogOps, ResourceLogOps is still there.
* It's very important that these callbacks shouldn't fail, | ||
* otherwise there can be a resource leak. | ||
*/ | ||
def observe( |
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.
are you sure that this is useful? :)
could you please describe your use case
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.
This is to add metrics. Adding metrics to a resource is very similar to adding logs.
see consumerWithMetrics
in another PR:
https://github.com/evolution-gaming/kafka-journal/pull/500/files#diff-fdca83b54e7278ff458b068b2b38db53c143ce7138c3074defce921a321ce425R40
consumerWithMetrics
looks pretty ugly, I want to avoid that
It's common to add logs and/or metrics to resource acquisition/release code. There's already a way to log how much time acquisition/release take -
ResourceLogOps.log
. But to add metrics we have to write code withallocated
again. It's error-prone, so I think it makes sense to have this logic in cats-helper. This PR adds a more generalobserve
method that allows to add callbacks for metrics and logs.The downside is that it's on user to make sure the callbacks can't fail. If a callback after a successful acquire or a callback before the release fail, there will be a resource leak.