Replace `SpanBuilder#wrapResource` API #273

NthPortal · 2023-07-07T16:10:09Z

Replace SpanBuilder#wrapResource with
SpanOps#resource:Resource[F,F~>F], which no longer automatically
provides 'acquire', 'use' and 'release' spans.

This API change allows further simplification of the API in future commits.

This PR is a follow-up to the discussion on #261.

rebase -i --autosquash before merge

NthPortal · 2023-07-07T16:10:29Z

cc: @iRevive @rossabaker @armanbilge

NthPortal · 2023-07-07T16:19:11Z

there is an argument that the entirety of SpanOps can now be discarded, with the new SpanOps#resource method moving to SpanBuilder and replacing build (name can be bikeshedded)

NthPortal · 2023-07-07T16:49:21Z

Looking at this some more, the only way to directly access the Span anymore is startUnmanaged. is that still needed? should it be moved? should SpanOps be kept just for that and resource?

iRevive · 2023-07-07T16:54:29Z

I'm firmly against removing SpanOps. SpanOps provides the only option to modify the span somewhere downstream.

For example, I can do the following:

tracer.span("my-span").use { span =>
  for {
    isSuccess <- someLogic
    _ <- span.addAttribute(Attribute("success", isSuccess))
  } yield ()
}

With F ~> F we cannot do such a thing.

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

iRevive · 2023-07-07T17:02:20Z

I like the direction. The moment we limited tracing to Local semantics, wrapResource does not offer much benefit.

If someone needs it, a similar tracing (acquire, use, release) can be achieved with a few lines utility method.

NthPortal · 2023-07-07T17:21:37Z

pushed a commit where resource now returns Resource[F, (Span[F], F ~> F)].

If someone needs it, a similar tracing (acquire, use, release) can be achieved with a few lines utility method.

TracerSuite currently defines such a method, so that I didn't have to mess with the test outputs.

I think I agree that it's still worth keeping SpanOps even with this change, however, as it allows a user to not have to faff about with the natural transformation. however, we can probably make the other methods final defs that delegate to resource (other than startUnmanaged).

edit: they cannot be final defs as things are because they require MonadCancelThrow. unless we're okay with requiring MonadCancelThrow as an implicit parameter for those? I definitely am

NthPortal · 2023-07-07T17:24:31Z

examples/src/main/scala/TraceExample.scala

+            .spanResource("Start up")
+            .use { case (_, nt /* natural transformation */ ) =>
+              for {
+                _ <- nt(tracer.span("acquire").surround(IO.sleep(50.millis)))
+                _ <- nt {
+                  tracer.span("use").surround {
+                    userIdAlg
+                      .getAllUsersForInstitution(
+                        "9902181e-1d8d-4e00-913d-51532b493f1b"
+                      )
+                      .flatMap(IO.println)
+                  }
+                }
+                _ <- nt(tracer.span("release").surround(IO.sleep(100.millis)))
+              } yield ()
+            }


I'm concerned that my original commit implemented this incorrectly (before I'd quite understood things), but nothing caught it. is there a way we can run the examples and make sure they're correct?

Hmm. sbt-doctest can make unit tests out of scaladoc comments, but I don't know of anything that tests the output of examples.

NthPortal · 2023-07-07T18:23:12Z

I've pretty much done the simplification (you can see it at https://github.com/NthPortal/otel4s/tree/Tracer-simplify/R1), but I'm holding off on PRing it until everything is hashed out in this PR

rossabaker · 2023-07-08T02:22:00Z

I'm firmly against removing SpanOps. SpanOps provides the only option to modify the span somewhere downstream.

In Natchez, there's a near 1:1 relationship between things you can do on a span and things you can do on a Tracer without a reference to the Span. To check my understanding, is that the basic tradeoff we've made here: we have to carry around a SpanOps but not mirror all the span operations?

rossabaker

Nice work!

core/trace/src/main/scala-2/org/typelevel/otel4s/trace/TracerMacro.scala

rossabaker · 2023-07-08T02:15:32Z

examples/src/main/scala/TraceExample.scala

+            .spanResource("Start up")
+            .use { case (_, nt /* natural transformation */ ) =>
+              for {
+                _ <- nt(tracer.span("acquire").surround(IO.sleep(50.millis)))
+                _ <- nt {
+                  tracer.span("use").surround {
+                    userIdAlg
+                      .getAllUsersForInstitution(
+                        "9902181e-1d8d-4e00-913d-51532b493f1b"
+                      )
+                      .flatMap(IO.println)
+                  }
+                }
+                _ <- nt(tracer.span("release").surround(IO.sleep(100.millis)))
+              } yield ()
+            }


Hmm. sbt-doctest can make unit tests out of scaladoc comments, but I don't know of anything that tests the output of examples.

examples/src/main/scala/TraceExample.scala

iRevive · 2023-07-08T09:00:27Z

In Natchez, there's a near 1:1 relationship between things you can do on a span and things you can do on a Tracer without a reference to the Span. To check my understanding, is that the basic tradeoff we've made here: we have to carry around a SpanOps but not mirror all the span operations?

Hm, it's a nice question. We can keep some conversation here #89, to make things easier

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

java/trace/src/test/scala/org/typelevel/otel4s/java/trace/TracerSuite.scala

iRevive

I like the current changes, thanks for the contribution!

There are a few leftovers (e.g. remove Result type member, etc) I can take care of.

NthPortal · 2023-07-17T16:15:24Z

There are a few leftovers (e.g. remove Result type member, etc) I can take care of.

too late ;)

core/trace/src/main/scala-2/org/typelevel/otel4s/trace/TracerMacro.scala

iRevive

Looks good. Only one change is up for discussion.

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

examples/src/main/scala/TracingExample.scala

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

NthPortal · 2023-07-17T17:39:31Z

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

@@ -105,9 +136,3 @@ trait SpanOps[F[_]] {
    */
  def surround[A](fa: F[A]): F[A]


this can still be final ... = use(_ => fa), will change momentarily

It's not final on Resource itself. I wonder whether that implies possible optimizations. But I can't imagine any offhand.

technically you can optimise the no-op implementation slightly by not creating a wrapper Function1 around the F[A] to pass to use, but honestly I still feel it's cleaner to have it be final. it can be made not final in a minor release if minds are changed later. or someone can make a PR after this is merged. or if others also want it non-final, I can add another fixup commit to this PR

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

NthPortal · 2023-07-21T16:59:06Z

if no one else has any objections within a couple days, I'll autosquash so this can be ready to merge

rossabaker

This is tremendous, both the code and review discussion. I should take vacations more often!

Replace `SpanBuilder#wrapResource` with `SpanOps#resource:Resource[F,F~>F]`, which no longer automatically provides 'acquire', 'use' and 'release' spans. This API change allows further simplification of the API in future commits.

Add `SpanOps.Res` type for the return type of `SpanOps#resource`, for clearer, more evolvable and more documentable API.

iRevive · 2023-07-24T13:05:09Z

Thanks for the contribution @NthPortal!

NthPortal · 2023-07-24T15:38:42Z

@iRevive thank you for reviewing this PR and collaborating with me!

NthPortal force-pushed the Tracer-spanResource/PR branch from 9da52cb to 35bd769 Compare July 7, 2023 16:15

NthPortal force-pushed the Tracer-spanResource/PR branch 2 times, most recently from ec21382 to e8b6044 Compare July 7, 2023 16:28

iRevive reviewed Jul 7, 2023

View reviewed changes

java/trace/src/main/scala/org/typelevel/otel4s/java/trace/SpanBuilderImpl.scala Outdated Show resolved Hide resolved

NthPortal commented Jul 7, 2023

View reviewed changes

rossabaker reviewed Jul 8, 2023

View reviewed changes

iRevive reviewed Jul 17, 2023

View reviewed changes

core/trace/src/main/scala/org/typelevel/otel4s/trace/SpanOps.scala Outdated Show resolved Hide resolved

iRevive reviewed Jul 17, 2023

View reviewed changes

java/trace/src/test/scala/org/typelevel/otel4s/java/trace/TracerSuite.scala Outdated Show resolved Hide resolved

iRevive approved these changes Jul 17, 2023

View reviewed changes

iRevive reviewed Jul 17, 2023

View reviewed changes

core/trace/src/main/scala-2/org/typelevel/otel4s/trace/TracerMacro.scala Outdated Show resolved Hide resolved

iRevive requested changes Jul 17, 2023

View reviewed changes

core/trace/src/main/scala/org/typelevel/otel4s/trace/SpanOps.scala Outdated Show resolved Hide resolved

examples/src/main/scala/TracingExample.scala Outdated Show resolved Hide resolved

NthPortal commented Jul 17, 2023

View reviewed changes

core/trace/src/main/scala/org/typelevel/otel4s/trace/Tracer.scala Outdated Show resolved Hide resolved

NthPortal requested a review from iRevive July 17, 2023 17:08

NthPortal commented Jul 17, 2023

View reviewed changes

iRevive reviewed Jul 17, 2023

View reviewed changes

core/trace/src/main/scala/org/typelevel/otel4s/trace/SpanOps.scala Outdated Show resolved Hide resolved

iRevive mentioned this pull request Jul 17, 2023

Tracing a Resource While Staying in Resource #194

Open

iRevive approved these changes Jul 17, 2023

View reviewed changes

rossabaker approved these changes Jul 21, 2023

View reviewed changes

Replace SpanBuilder#wrapResource API

94a5487

Replace `SpanBuilder#wrapResource` with `SpanOps#resource:Resource[F,F~>F]`, which no longer automatically provides 'acquire', 'use' and 'release' spans. This API change allows further simplification of the API in future commits.

NthPortal added 2 commits July 24, 2023 07:26

Simplify Tracer and Span{Builder,Ops}

b5ca781

Change return type of SpanOps#resource

caeea8d

Add `SpanOps.Res` type for the return type of `SpanOps#resource`, for clearer, more evolvable and more documentable API.

NthPortal force-pushed the Tracer-spanResource/PR branch from b50c18c to caeea8d Compare July 24, 2023 11:26

iRevive merged commit 5a90aac into typelevel:main Jul 24, 2023

NthPortal deleted the Tracer-spanResource/PR branch July 24, 2023 15:38

NthPortal mentioned this pull request Jul 24, 2023

Add Tracer.translate #264

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `SpanBuilder#wrapResource` API #273

Replace `SpanBuilder#wrapResource` API #273

NthPortal commented Jul 7, 2023 •

edited

Loading

NthPortal commented Jul 7, 2023

NthPortal commented Jul 7, 2023 •

edited

Loading

NthPortal commented Jul 7, 2023

iRevive commented Jul 7, 2023 •

edited

Loading

iRevive commented Jul 7, 2023

NthPortal commented Jul 7, 2023 •

edited

Loading

NthPortal Jul 7, 2023

rossabaker Jul 8, 2023

NthPortal commented Jul 7, 2023

rossabaker commented Jul 8, 2023

rossabaker left a comment

rossabaker Jul 8, 2023

iRevive commented Jul 8, 2023

iRevive left a comment •

edited

Loading

NthPortal commented Jul 17, 2023

iRevive left a comment

NthPortal Jul 17, 2023

rossabaker Jul 21, 2023

NthPortal Jul 22, 2023

NthPortal commented Jul 21, 2023

rossabaker left a comment

iRevive commented Jul 24, 2023

NthPortal commented Jul 24, 2023 •

edited

Loading

		@@ -105,9 +136,3 @@ trait SpanOps[F[_]] {
		*/
		def surround[A](fa: F[A]): F[A]

Replace SpanBuilder#wrapResource API #273

Replace SpanBuilder#wrapResource API #273

Conversation

NthPortal commented Jul 7, 2023 • edited Loading

NthPortal commented Jul 7, 2023

NthPortal commented Jul 7, 2023 • edited Loading

NthPortal commented Jul 7, 2023

iRevive commented Jul 7, 2023 • edited Loading

iRevive commented Jul 7, 2023

NthPortal commented Jul 7, 2023 • edited Loading

NthPortal Jul 7, 2023

Choose a reason for hiding this comment

rossabaker Jul 8, 2023

Choose a reason for hiding this comment

NthPortal commented Jul 7, 2023

rossabaker commented Jul 8, 2023

rossabaker left a comment

Choose a reason for hiding this comment

rossabaker Jul 8, 2023

Choose a reason for hiding this comment

iRevive commented Jul 8, 2023

iRevive left a comment • edited Loading

Choose a reason for hiding this comment

NthPortal commented Jul 17, 2023

iRevive left a comment

Choose a reason for hiding this comment

NthPortal Jul 17, 2023

Choose a reason for hiding this comment

rossabaker Jul 21, 2023

Choose a reason for hiding this comment

NthPortal Jul 22, 2023

Choose a reason for hiding this comment

NthPortal commented Jul 21, 2023

rossabaker left a comment

Choose a reason for hiding this comment

iRevive commented Jul 24, 2023

NthPortal commented Jul 24, 2023 • edited Loading

Replace `SpanBuilder#wrapResource` API #273

Replace `SpanBuilder#wrapResource` API #273

NthPortal commented Jul 7, 2023 •

edited

Loading

NthPortal commented Jul 7, 2023 •

edited

Loading

iRevive commented Jul 7, 2023 •

edited

Loading

NthPortal commented Jul 7, 2023 •

edited

Loading

iRevive left a comment •

edited

Loading

NthPortal commented Jul 24, 2023 •

edited

Loading