Skip to content

SpanBuilder: replace startResource with wrapResource #86

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

Merged
merged 2 commits into from
Feb 7, 2023

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Jan 23, 2023

Currently, there are three options to start a span:

trait SpanBuilder[F[_]] {
  def startUnmanaged: F[Span[F]]
  def start: Resource[F, Span[F]]
  def startResource[A](resource: Resource[F, A]): Resource[F, Span.Res[F]]
}

startResource bothers me a bit.

Rather than providing startResource, we can introduce wrapResource and utilize an abstract type to infer the output.

With these changes, there are only two options to start the span.
The result type is inferred automatically:

val span: Resource[F, Span[F]] = tracer.spanBuilder("test").start
val span: Resource[F, Span.Res[F, String]] = tracer.spanBuilder("test").wrapResource(Resource.pure("test")).start

The API change is the following:

// before
tracer.spanBuilder("test").startResource(Resource.pure("test")).use { case span @ Span.Res(value) =>
  ???
}

// after
tracer.spanBuilder("test").wrapResource(Resource.pure("test")).start.use { case span @ Span.Res(value) =>
  ???
}

Pros:

  1. Two fixed options to start the span
  2. Due to type constraints, you cannot call startUnmanaged or wrapResource on a builder that wraps a resource
  3. start infers the result type automatically

Cons:

  1. The API may be confusing
  2. Perhaps the type inference may be tricky in some cases

P.S. The implementation of Runner is dirty. If we agree on design changes, I will provide a more elegant one.

@iRevive iRevive force-pushed the span-builder-wrap-resource branch from eeb3d64 to 8035120 Compare January 23, 2023 08:59
@rossabaker
Copy link
Member

This seems like a good idea. We need to write some real world things to find the real world inference, but I'm in favor of giving it a try.

@zmccoy zmccoy self-requested a review January 25, 2023 16:18
@zmccoy
Copy link
Member

zmccoy commented Jan 26, 2023

One note is that when I first read this and saw .start I immediately went to spinning a fiber off, which isn't what this is but in the eco system I feel like that method name may make people think we're dealing with a different type than it is without closer context reading.

@iRevive
Copy link
Contributor Author

iRevive commented Jan 27, 2023

@zmccoy valid point.

What else can we use? startSpan? There was create option at some point #37 (comment)

@zmccoy
Copy link
Member

zmccoy commented Jan 30, 2023

@iRevive Sorry for the late reply, I think startSpan is clear and not confusing.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

I was working on #107, but I don't see any harm in merging this before it rots.

@iRevive iRevive merged commit 097d053 into typelevel:main Feb 7, 2023
@iRevive iRevive deleted the span-builder-wrap-resource branch February 7, 2023 09:28
chr12c pushed a commit to chr12c/otel4s that referenced this pull request Jun 26, 2023
SpanBuilder: replace `startResource` with `wrapResource`
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