Skip to content

Conversation

@DocCringol
Copy link

@DocCringol DocCringol commented May 16, 2025

I've seen the implementation of mapK for Tracer and thought of doing the same for Meter.
I don't pretend that what I'm suggesting is a good implementation though. Also I didn't really get where is the best place to implement tests. So I did only an example one, for counter

Also, there is a point could be made that it is possible to make less amount of boilerplate code (in observable components at least) by encapsulating Builder trait

So, it's more of a feature suggestion

But if somebody will like the idea and will be able to tell me more about project structure then I'm ready to finish this pr

@mergify mergify bot added module:core Features and improvements to core module module:sdk Features and improvements to the sdk module metrics Improvement to metrics module labels May 16, 2025
@iRevive
Copy link
Contributor

iRevive commented May 20, 2025

Thanks for the PR! I will review the changes this week.


import cats.Applicative
import cats.Functor
import cats.implicits.toFunctorOps
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes things slightly more obvious:

Suggested change
import cats.implicits.toFunctorOps
import cats.syntax.functor._

We can apply the same change in all places.

@iRevive iRevive requested a review from NthPortal May 26, 2025 18:01
@iRevive
Copy link
Contributor

iRevive commented May 26, 2025

Looks good overall, thanks!

@NthPortal would you like to take a look?

Copy link
Contributor

@NthPortal NthPortal left a comment

Choose a reason for hiding this comment

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

this feature has a fundamental problem, which is that KindTransformers (perhaps better named LiftKind or KindLifter) do not (with the exception of the identity case) work in reverse.

as an example, one of the most common types for G is OptionT[F, *] (i.e. a wrapper around F[Option[*]]); you cannot unlift OptionT[F, *] back to F, because F[None] has no value to unwrap/unlift

Comment on lines +294 to +296
mct: MonadCancelThrow[F],
kt: KindTransformer[F, G],
tk: KindTransformer[G, F]
Copy link
Contributor

@NthPortal NthPortal May 27, 2025

Choose a reason for hiding this comment

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

just some name bikeshedding:

  • monadic constraints tend to just share the name of the HKT (idk why, but it's conventional)
  • kt is just the first letters of 'Kind' and 'Transformer', so tk doesn't make a lot of sense as a name
Suggested change
mct: MonadCancelThrow[F],
kt: KindTransformer[F, G],
tk: KindTransformer[G, F]
F: MonadCancelThrow[F],
ktFG: KindTransformer[F, G],
ktGF: KindTransformer[G, F]

for {
state <- InMemoryMeterSharedState.create[IO](resource, scope, 0.seconds)
builder = SdkCounter.Builder[IO, Long](name, state.state, None, None)
mappedBuilder = builder.mapK[IO]
Copy link
Contributor

Choose a reason for hiding this comment

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

this is an identity transformation


/** Modify the context `F` using an implicit [[KindTransformer]] from `F` to `G`.
*/
def mapK[G[_]: MonadCancelThrow](implicit
Copy link
Member

Choose a reason for hiding this comment

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

I think when both F ~> G and G ~> F are required, the method is more typically named imapK, e.g. in InvariantK from cats-tagless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

metrics Improvement to metrics module module:core Features and improvements to core module module:sdk Features and improvements to the sdk module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants