Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Local-compatible tracing semantics #107

Merged
merged 21 commits into from
Feb 7, 2023
Merged

Local-compatible tracing semantics #107

merged 21 commits into from
Feb 7, 2023

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Feb 2, 2023

No description provided.

rossabaker and others added 6 commits January 31, 2023 08:44
@rossabaker
Tracing with local semantics
5fb8172
@rossabaker
Add surround
4525c85
@rossabaker
Revive span macro
2ef92ef
@rossabaker
Merge remote-tracking branch 'origin/main' into local-tracing
2a20646
@iRevive
Merge branch 'span-builder-wrap-resource' into local-tracing-span-bui…
47680da 
…lder

# Conflicts:
#	core/trace/src/main/scala/org/typelevel/otel4s/trace/SpanBuilder.scala
#	core/trace/src/main/scala/org/typelevel/otel4s/trace/Tracer.scala
#	java/trace/src/main/scala/org/typelevel/otel4s/java/trace/SpanBuilderImpl.scala
@iRevive
Merge local traces
e3f9bb2
@iRevive iRevive marked this pull request as draft February 2, 2023 17:50
@rossabaker
Copy link
Member

This is neat.

Positives:

  • We get local semantics. I think we could implement a TracerScope from cats.mtl.Local, even if IOLocal is really the most compelling one.
  • wrapResource lets us span acquire/use/release together. This has been difficult to accomplish with local semantics. Natchez can't do it.
  • I can't break Stream like I did on Tracing with local semantics #105.

Tradeoffs:

  • wrapResource doesn't return a Resource, but an Aux type with resource-like methods. The end API still looks nice, but it may be unwieldy in deeper resource stacks?
  • Because it doesn't return a Resource, it's not obvious how to span a Stream. The breakage from Tracing with local semantics #105 was fixed beacuse it doesn't compile. I think Stream would need special support like Resource.

My intuition is that we should only need two span types: one for MonadCancelThrow, where flatMap happens outside release, and one for Stream/Resource like things, where flatMap happens inside release.

rossabaker added a commit to rossabaker/skunk that referenced this pull request Feb 2, 2023
@rossabaker
Rebuild on typelevel/otel4s#107
49b412f
@rossabaker
Copy link
Member

And here's "the skunk test". Only one change, but it means the bar for noop is raised from Applicative to MonadCancelThrow. But it doesn't bring back #85, so I don't think that's a huge problem.

iRevive
iRevive commented Feb 3, 2023
View reviewed changes
core/trace/src/main/scala/org/typelevel/otel4s/trace/SpanBuilder.scala Outdated
Applicative[F].pure(span)

def use[A](f: Res => F[A]): F[A] =
startSpan.use(res => f(res))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need MonadCancelThrow there

rossabaker
rossabaker reviewed Feb 6, 2023
View reviewed changes
build.sbt Outdated Show resolved Hide resolved
@rossabaker rossabaker mentioned this pull request Feb 7, 2023
Closed
rossabaker
rossabaker reviewed Feb 7, 2023
View reviewed changes
java/trace/src/test/scala/org/typelevel/otel4s/java/trace/TracerSuite.scala
@@ -472,6 +471,7 @@ class TracerSuite extends CatsEffectSuite {
}
}

/*
Copy link
Member

Choose a reason for hiding this comment

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

We've lost the ability to trace streams in this API. I believe I've made a Local[Stream[F, *], E], so we should be able to get it back, but this PR is already getting large.

@rossabaker rossabaker changed the title Local tracing span builder Local-compatible tracing semantics Feb 7, 2023
@rossabaker rossabaker marked this pull request as ready for review February 7, 2023 03:40
@rossabaker
Copy link
Member

This isn't perfect, but if we're committed to cats.mtl.Local semantics as the basis, this is the best thing we've got right now.

@rossabaker
Copy link
Member

Work in progress of natchez-http4s-otel: rossabaker/natchez-http4s-otel#1. Translating EntryPoint and Kernel are the major sticking points.

@iRevive
Copy link
Contributor Author

iRevive commented Feb 7, 2023

I merged the upstream and updated some ScalaDocs.

rossabaker
rossabaker approved these changes Feb 7, 2023
View reviewed changes
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.

Weird approval because it's a joint effort but I think we should land this and iterate from here.

@rossabaker rossabaker mentioned this pull request Feb 7, 2023
Closed
@iRevive
Copy link
Contributor Author

iRevive commented Feb 7, 2023

Sounds good. I want to clean up my mess in SpanBuilderImpl from #86.

@iRevive iRevive merged commit f0c9683 into typelevel:main Feb 7, 2023
@iRevive iRevive deleted the local-tracing-span-builder branch February 7, 2023 13:17
@rossabaker rossabaker mentioned this pull request Feb 7, 2023
Merged
@iRevive iRevive mentioned this pull request May 3, 2023
Open
chr12c pushed a commit to chr12c/otel4s that referenced this pull request Jun 26, 2023
@iRevive
Merge pull request typelevel#107 from iRevive/local-tracing-span-builder
564a794 
Local-compatible tracing semantics
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rossabaker rossabaker rossabaker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@iRevive @rossabaker