Move SpanRunner outside SpanBuilderImpl
#122
Conversation
| @@ -89,7 +90,7 @@ private[java] class SpanBackendImpl[F[_]: Sync]( | |||
| } | |||
|
|
|||
| private[otel4s] def end: F[Unit] = | |||
| Sync[F].delay(jSpan.end()) | |||
| Sync[F].realTime.flatMap(now => end(now)) | |||
There was a problem hiding this comment.
Technically, only startUnmanaged is affected. For resource-like API, we were injecting the realtime under the hood. E.g.
https://github.com/typelevel/otel4s/pull/122/files#diff-988bfe14df2f23c008240b2752bde4a711b0c4fa716b62f5416f1de82582bc55L330-L333
| * | ||
| * @see | ||
| * See [[use]] for more details regarding lifecycle strategy | ||
| */ |
There was a problem hiding this comment.
Ah, thank you for adding these, too!
| mimaBinaryIssueFilters ++= Seq( | ||
| ProblemFilters.exclude[MissingClassProblem]( | ||
| "org.typelevel.otel4s.java.trace.SpanBuilderImpl$Runner" | ||
| ), | ||
| ProblemFilters.exclude[MissingClassProblem]( | ||
| "org.typelevel.otel4s.java.trace.SpanBuilderImpl$Runner$" | ||
| ), | ||
| ProblemFilters.exclude[MissingClassProblem]( | ||
| "org.typelevel.otel4s.java.trace.SpanBuilderImpl$TimestampSelect" | ||
| ), | ||
| ProblemFilters.exclude[MissingClassProblem]( | ||
| "org.typelevel.otel4s.java.trace.SpanBuilderImpl$TimestampSelect$" | ||
| ), | ||
| ProblemFilters.exclude[MissingClassProblem]( | ||
| "org.typelevel.otel4s.java.trace.SpanBuilderImpl$TimestampSelect$Delegate$" | ||
| ), | ||
| ProblemFilters.exclude[MissingClassProblem]( | ||
| "org.typelevel.otel4s.java.trace.SpanBuilderImpl$TimestampSelect$Explicit$" | ||
| ) |
There was a problem hiding this comment.
None of this code would be inlined by the macros, right? This just bit Circe on Scala 3...
I have a feeling our next release will be 0.2, but as long as we're not changing the public API, this is a noble effort.
There was a problem hiding this comment.
No, these classes shouldn't appear in the inline positions
|
Is this our first PR in the MiMa world? I'm unsure what the Scala 3 build is cranky about there. |
|
Yes, as far as I am aware, it's the first PR after the 0.1.0 release |
|
I believe you are running into this issue, because you are publishing "aggregate" modules which don't contain classfiles. |
|
We could unblock this by emptying the mimaPreviousArtifacts, as long as we remember to roll it back before either:
|
|
|
Yeah, the next release will contain new features, and nobody is using this in prod yet ... at least hopefully not for libraries! 0.2 is fine with me, too. |
|
I should bump the base version to 0.2 in this PR, right? |
|
That only postpones the problem until after 0.2 is released, but I'm hopeful the problem will be gone by other means then, and it's a reasonable thing to do on its own. 👍 |
Move `SpanRunner` outside `SpanBuilderImpl`
While cleaning up the internal implementation, I spotted a few inconsistencies regarding start/end time selections.
Before (main branch):
ops.use. builder has start timeClock[F].realTimeops.useClock[F].realTimeClock[F].realTimeops.startUnmanaged. builder has start timeops.startUnmanagedAfter (this branch):
ops.use. builder has start timeClock[F].realTimeops.useClock[F].realTimeClock[F].realTimeops.startUnmanaged. builder has start timeClock[F].realTimeops.startUnmanagedClock[F].realTimeClock[F].realTimeWe can also introduce
TimestampStrategyto allow choosing betweenClock[F].realTimeandj_span.start / j_span.end.It feels like overkill, but it may be helpful in applications when otel4s and some Java libs use the same OpenTelementry SDK.