upgrade to natchez 0.3.x #101
Conversation
|
Hi - it would be great to see this make it through, it's starting to age out. :) |
|
@voidcontext is there any chance of publishing this, even as a milestone release? Otherwise I'm going to have to fork this and publish internally which makes me feel bad :) |
|
@barryoneill @massimosiani |
|
@voidcontext very much appreciated, thank you! |
|
HI @massimosiani , first of all thank you for keeping this PR up to date! We allocated some time in the coming weeks, to get this PR reviewed and merged. Meanwhile it seems there are some merge conflicts, if you could have a look at those that'd be cool. |
massimosiani
force-pushed
the
natchez-0.3.x
branch
from
5717aa2
to
1d65b76
Compare
|
Done! Thanks for your efforts on the project! |
There was a problem hiding this comment.
Hi @massimosiani, we started the review, there are some initial comments. We are planning to continue later this week.
| @@ -63,7 +63,7 @@ lazy val metricsCommon = projectMatrix | |||
| .settings(common :+ (name := "natchez-extras-metrics")) | |||
|
|
|||
| val log4catsVersion = "2.2.0" | |||
| val natchezVersion = "0.1.6" | |||
| val natchezVersion = "0.3.3" | |||
There was a problem hiding this comment.
natchez 0.3.3 is compiled with Scala 3.3.0 which means we need to bump our version as well (see compilation errors in github actions)
| @@ -16,7 +18,7 @@ class IOLocalTrace(private val local: IOLocal[Span[IO]]) extends Trace[IO] { | |||
| private def scope[G](t: Span[IO])(f: IO[G]): IO[G] = | |||
| MonadCancel[IO, Throwable].bracket(local.getAndSet(t))(_ => f)(local.set) | |||
|
|
|||
| override def span[A](name: String)(k: IO[A]): IO[A] = | |||
| override def span[A](name: String, options: Span.Options)(k: IO[A]): IO[A] = | |||
| local.get.flatMap { | |||
| _.span(name).use { span => | |||
There was a problem hiding this comment.
options should be passed here.
There was a problem hiding this comment.
@massimosiani We added a few more comments, other than those we realised the use of Span.Options is missing in some cases, but that's not blocking this PR, we can make use of those in follow up changes.
Once those issues, raised in comments are fixed, we're happy to approve this PR, thanks for your contribution!
| @@ -52,7 +61,7 @@ object Slf4jSpan { | |||
| Sync[F] | |||
| .fromEither( | |||
| k.toHeaders | |||
| .find(_._1.toLowerCase == "x-trace-token") | |||
| .find(_._1.toString.toLowerCase == "x-trace-token") | |||
There was a problem hiding this comment.
This should be simplified to
.find(_._1 == ci"x-trace-token")| @@ -46,6 +47,11 @@ object TestEntryPoint { | |||
| def spanId: F[Option[String]] = F.pure(None) | |||
| def traceUri: F[Option[URI]] = F.pure(None) | |||
| def kernel: F[Kernel] = F.pure(kern) | |||
| override def log(fields: (String, TraceValue)*): F[Unit] = put(fields: _*) | |||
There was a problem hiding this comment.
It's OK to add the override keyword as this is something we might want to do in the future, but we need to be consistent, so either add it everywhere in this file, or we don't add it at all.
massimosiani
force-pushed
the
natchez-0.3.x
branch
from
3830888
to
4ca1374
Compare
Hi!
I'm trying to bump natchez to the latest version.
I'm unsure whether there are binary incompatibilities here.
Also, I'm not an expert in Datadog tracing, so I'd need a double check on that part.