Add `Tracer.translate` #264

iRevive · 2023-07-03T17:39:43Z

Proof of concept for #261

iRevive · 2023-07-03T17:41:12Z

core/trace/src/main/scala/org/typelevel/otel4s/trace/Tracer.scala

@@ -172,6 +174,8 @@ trait Tracer[F[_]] extends TracerMacro[F] {
    */
  def noopScope[A](fa: F[A]): F[A]

+  def mapK[G[_]: Sync](fk: F ~> G, gk: G ~> F): Tracer[G]


Here the type constraint must satisfy SpanRunner and TraceScope from the java-trace package.

armanbilge · 2023-07-03T17:44:21Z

core/trace/src/main/scala/org/typelevel/otel4s/trace/Tracer.scala

@@ -172,6 +174,8 @@ trait Tracer[F[_]] extends TracerMacro[F] {
    */
  def noopScope[A](fa: F[A]): F[A]

+  def mapK[G[_]: Sync](fk: F ~> G, gk: G ~> F): Tracer[G]


if you need both F ~> G and G ~> F then actually it's an imapK 😜 also sometimes the method is called translate.

Also, Sync is a very strong constraint, and I don't think it's necessary. Once you have F ~> G and G ~> F then you should be able to derive Sync[G] from Sync[F].

See a similar idea in:

Fix Client#translate and relax constraints http4s/http4s#6139

translate is more suitable indeed.

Once you have F ~> G and G ~> F then you should be able to derive Sync[G] from Sync[F].

I didn't know that! I will take a look at the example, looks interesting.

If you need F ~> G and G ~> F, then the invariant nature means almost all interesting transformers are eliminated from use, OptionT, EitherT, Kleisli, IorT, etc.

Random thought: you might be able to do this with something weaker than F ~> G + G ~> F, like Haskell's UnliftIO. That only gets you Kleisli while staying sensible, but it's something at least.

Prototype: Prillan@355c68d

armanbilge · 2023-07-03T17:57:40Z

java/trace/src/main/scala/org/typelevel/otel4s/java/trace/TracerImpl.scala

+
+object TracerImpl {
+
+  private def liftSync[F[_], G[_]](


Nice! We might consider upstreaming these methods to Cats Effect is they are generally useful.

That's what I was thinking about too

# Conflicts: # core/trace/src/main/scala/org/typelevel/otel4s/trace/Tracer.scala

iRevive · 2023-07-24T14:53:04Z

I remembered the implicit class XXXSyntax[F[_]](...) trick, so the API is slightly cleaner and does not leak into the implementation.

A few notes:

There is a generic API mapK and translate
For some components, mapK cannot be implemented, since the fg: G ~> F is still required. So I added translate instead.
liftOptionT requires a few copy-pastes, but compiles fine

iRevive · 2023-07-24T14:57:54Z

@NthPortal - would you like to take a look? These changes intersect with your #261 PR.

NthPortal · 2023-07-24T16:25:32Z

core/trace/src/main/scala/org/typelevel/otel4s/trace/SpanBuilder.scala

+  implicit final class SpanBuilderSyntax[F[_]](
+      private val builder: SpanBuilder[F]
+  ) extends AnyVal {
+
+    def translate[G[_]](fk: F ~> G, gk: G ~> F)(implicit
+        F: MonadCancel[F, _],
+        G: MonadCancel[G, _]
+    ): SpanBuilder[G] =


is the reason for this so that the primary API doesn't contain MonadCancel?

(side note: I thought it needed MonadCancelThrow specifically)

Hmm, do we really need both MonadCancel for F and G? If we have both fk and gk seems like we should be able to derive it.

is the reason for this so that the primary API doesn't contain MonadCancel?

We need it in several places, for example: https://github.com/typelevel/otel4s/pull/264/files#diff-36bdd6d74392dca0ddc0d9c6312f06c778e77a49e87fbc5c6fc966c936171f1cR208

Hmm, do we really need both MonadCancel for F and G? If we have both fk and gk seems like we should be able to derive it.

Indeed

We need it in several places

oops, I worded that question really poorly. what's the reason it's an extension method and not in the trait?

That way I do not need to deal with recursive definitions, e.g.:

trait Tracer[F[_]] { self => def mapK[G[_]](fk: F ~> G): Tracer[G] = new Tracer { ... def mapK[Q[_]](fk1: G ~> Q): Tracer[Q] = self.mapK(fk.andThen(fk1))) } }

I like your approach with a dedicated type MappedK way more.

NthPortal · 2023-07-24T16:41:53Z

If you need F ~> G and G ~> F, then the invariant nature means almost all interesting transformers are eliminated from use, OptionT, EitherT, Kleisli, IorT, etc.

liftOptionT requires a few copy-pastes, but compiles fine

I actually worked on a solution to both of these problems while working on #273; perhaps it can be combined with this PR for a union of the features of the two. I haven't quite finished the scaladocs, but I've pushed up my work as #284

bpholt · 2023-07-24T16:48:28Z

FWIW, cats-tagless has some laws for FunctorK and InvariantK that might apply here. (It'd be nice to also provide no-import instances for those typeclasses where possible, but I understand that cats-tagless may not be stable enough to include as a dependency.)

NthPortal · 2023-07-24T17:20:26Z

core/trace/src/main/scala/org/typelevel/otel4s/trace/SpanOps.scala

+      def translate[G[_]](fk: F ~> G, gk: G ~> F): Res[G] =
+        new Res[G] {
+          def span: Span[G] = res.span.mapK(fk)
+          def trace: G ~> G = res.trace.andThen(fk).compose(gk)
+        }


is there a reason not to use Res.apply for this?

trace at least should probably be a val because if you don't use it, nothing ends up traced, so it will be used in almost all cases

Yeah, Res.apply fits better

NthPortal · 2023-07-27T01:02:57Z

I like translate and mapK, and the derivation of MonadCancelThrow for translate is very clever as well. my main concern is the amount of code duplication that would be needed in order to support transformations to types other than OptionT

NthPortal · 2023-08-23T20:36:20Z

@iRevive how do you want to reconcile our two PRs? do you want me to rewrite the parts of mine on top of yours, or yours on top of mine, or something else?

iRevive · 2023-08-24T07:45:38Z

@NthPortal I prefer your version. It's more flexible, in my opinion.

I'll close this PR in favor of #284.

Add Tracer.mapK

16716c9

iRevive commented Jul 3, 2023

View reviewed changes

iRevive mentioned this pull request Jul 3, 2023

[WIP] Mostly complete Tracer impl for F -> OptionT #261

Closed

armanbilge reviewed Jul 3, 2023

View reviewed changes

Rename mapK -> translate

fbb2f04

iRevive changed the title ~~Add Tracer.mapK~~ Add Tracer.translate Jul 3, 2023

iRevive added 2 commits July 3, 2023 20:53

Relax type constraint

65f5f56

Remove type constraint

102c42f

armanbilge reviewed Jul 3, 2023

View reviewed changes

Merge branch 'upstream-main' into tracer-mapK

4139fdb

# Conflicts: # core/trace/src/main/scala/org/typelevel/otel4s/trace/Tracer.scala

iRevive force-pushed the tracer-mapK branch from dc7038d to 5a12d7f Compare July 24, 2023 14:49

iRevive force-pushed the tracer-mapK branch from 5a12d7f to b191c1d Compare July 24, 2023 14:53

iRevive marked this pull request as ready for review July 24, 2023 14:56

iRevive requested review from armanbilge and rossabaker July 24, 2023 14:56

Add translate and mapK API to the tracing components

bfd4330

iRevive force-pushed the tracer-mapK branch from b191c1d to bfd4330 Compare July 24, 2023 15:01

NthPortal reviewed Jul 24, 2023

View reviewed changes

NthPortal mentioned this pull request Jul 24, 2023

Add mapK to Tracer, SpanBuilder, etc. #284

Merged

2 tasks

Derive MonadCancelThrow[G]

b31b7ae

NthPortal reviewed Jul 24, 2023

View reviewed changes

iRevive closed this Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Tracer.translate` #264

Add `Tracer.translate` #264

iRevive commented Jul 3, 2023

iRevive Jul 3, 2023

armanbilge Jul 3, 2023

iRevive Jul 3, 2023

ChristopherDavenport Jul 3, 2023

Prillan Jul 6, 2023

armanbilge Jul 3, 2023

iRevive Jul 3, 2023

iRevive commented Jul 24, 2023 •

edited

Loading

iRevive commented Jul 24, 2023

NthPortal Jul 24, 2023

armanbilge Jul 24, 2023

iRevive Jul 24, 2023

iRevive Jul 24, 2023

NthPortal Jul 24, 2023

iRevive Jul 31, 2023

NthPortal commented Jul 24, 2023

bpholt commented Jul 24, 2023

NthPortal Jul 24, 2023

NthPortal Jul 24, 2023

iRevive Jul 31, 2023

NthPortal commented Jul 27, 2023

NthPortal commented Aug 23, 2023

iRevive commented Aug 24, 2023

Add Tracer.translate #264

Add Tracer.translate #264

Conversation

iRevive commented Jul 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iRevive commented Jul 24, 2023 • edited Loading

iRevive commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthPortal commented Jul 24, 2023

bpholt commented Jul 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NthPortal commented Jul 27, 2023

NthPortal commented Aug 23, 2023

iRevive commented Aug 24, 2023

Add `Tracer.translate` #264

Add `Tracer.translate` #264

iRevive commented Jul 24, 2023 •

edited

Loading