X-Ray by christiankjaer · Pull Request #427 · typelevel/natchez

christiankjaer · 2021-09-27T11:09:11Z

Will serialize segments as described here and send them to the running X-Ray daemon as described here

Will not send child spans before the parent finishes.

bpholt · 2021-10-29T05:36:01Z

This is pretty cool! I really want to use Natchez with AWS Lambda and X-Ray, and this seems more promising than trying to integrate with the X-Ray SDK, which (as you probably know) uses ThreadLocals on the JVM, and so is hard to make work correctly with cats-effect code.

Instead of using the JVM's DatagramSocket and InetAddress classes in the implementation, have you given any thought to using fs2's DatagramSocket and ip4s's Host or IpAddress? Those are both pure implementations (whereas I think there are some side effects not wrapped in Sync[F].delay here) and available for both JVM and Scala.js.

christiankjaer · 2021-10-29T06:45:57Z

Hi @bpholt
That is definitely an option. I was just happy to get something working. I will try to update the implementation to make it a little more cats-y this weekend. Then maybe you will take a look at it, and we can get it merged so people can try it out?

bpholt · 2021-11-09T20:06:41Z

@tpolecat When you get a chance, would you mind approving the GitHub Actions workflow for this PR? It'd be nice to get that feedback.

armanbilge

Just some nits :)

armanbilge · 2021-11-10T12:48:32Z

+) extends EntryPoint[F] {
+
+  def sendSegment(foo: JsonObject): F[Unit] = {
+    val payload = (XRayEntryPoint.header + foo.asJson.noSpaces).getBytes()


Just a nit but I'm a big fan of scodec-bits methods for this e.g. encodeAscii or encodeUtf8:
https://github.com/scodec/scodec-bits/blob/main/core/shared/src/main/scala/scodec/bits/ByteVector.scala#L2105

Also, if you encode the header and json separately bytevector concatenation is faster than string concatenation :)

armanbilge · 2021-11-10T12:58:08Z

+          "exceptions" -> Json.arr(
+            Json.obj(
+              "id" -> id.asJson,
+              "message" -> ex.getMessage.asJson,
+              "type" -> ex.getClass.getName.asJson,
+              "stack" -> ex
+                .getStackTrace
+                .map(x =>
+                  Json.obj(
+                    "line" -> x.getLineNumber.asJson,
+                    "path" -> x.getFileName.asJson,
+                    "label" -> x.getMethodName.asJson
+                  )
+                )
+                .asJson


Maybe worth factoring this out into an Encoder[Exception] or something?

Well. The current design has the child segments as JsonObjects. Doing this in a more typed way requires a bigger refactor.

I mean, it doesn't need to be anything fancy. It's like declaring it its own method? Just wrap it up in an Encoder :)

Isn't encoders implicitly encoding Json? Then I would need to convert it to a JsonObject again which might return None

https://circe.github.io/circe/api/io/circe/Encoder$$AsObject.html

tpolecat · 2021-11-10T15:20:42Z

Since it looks like you're still working on this I'm going to change it to a draft. Click the thing when it's ready and it looks like we can get it merged quickly. Thanks!

…stem environment (which is useful in Lambda functions)

…environment

christiankjaer · 2021-11-14T11:53:44Z

@armanbilge @bpholt Could one of you guys take a quick look again before I set this as ready?

armanbilge

Personally I'd like to see #427 (comment) and #427 (comment) addressed. But I'm not a maintainer so no stake here :)

armanbilge · 2021-11-14T12:59:44Z

+    "Root=1-5759e988-bd862e3fe1be46a994272793;Parent=53995c3f42cd8ad8;Sampled=1"
+  val notSampled = "Root=1-5759e988-bd862e3fe1be46a994272793;Sampled=0"
+
+  test("header parsing") {


Maybe overkill, but since you have both header parsing/encoding we could have a scalacheck prop testing the roundtrip.

I thought about it. Came to the conclusion that it would be overkill.

FWIW I also think it's a good idea, but IMO it can come in a followup PR if someone wants to do it. Similarly there are places where it might be more clear to use JSON literals instead of constructing JSON objects from key/value pairs, but I think it'd be fine to do that later too.

Yes I had the same thought about JSON literals :)

armanbilge

Looking good to me, thanks for addressing all my feedback! Apologies since we dragged this a bit out from your original scope, but much appreciated 😁

christiankjaer · 2021-11-14T20:36:41Z

@armanbilge Right. To give some context, the original implementation was just a simple adaption of the logging implementation, so the design choices are just stolen.

christiankjaer · 2021-11-16T11:29:37Z

@tpolecat
I think the arguments are all over now. You can merge it if you think it's alright.
Thanks!

bpholt · 2021-11-17T00:13:22Z

+
+      override def kernelFromEnvironment: F[Kernel] =
+        OptionT(traceId)
+          .map(XRaySpan.XRayHeader(_, None, sampled = true).toKernel)


I think I got this wrong in my pull request. It looks like traceId returns something like Root=1-619446d4-2de198a61485ba3d50bf889d;Parent=7c143d692eca32b3;Sampled=1 if _X_AMZN_TRACE_ID is present in the environment, meaning the value should be parsed instead of just dropped into a new XRayHeader directly.

Instead, I think this works:

OptionT(traceId) .subflatMap(XRaySpan.parseHeader) .map(_.toKernel) .getOrElse(Kernel(Map.empty))

bpholt · 2021-11-17T00:22:26Z

+      .get(Header)
+      .flatMap(parseHeader)
+      .map(x => fromHeader(name, x, entry))
+      .get


I think this .get is unsafe because it may throw an exception outside the scope of the F[_] effect. One option would be to do something like this:

OptionT.fromOption[F](kernel.toHeaders.get(Header)) .subflatMap(parseHeader) .semiflatMap(fromHeader(name, _, entry)) .getOrElseF(new NoSuchElementException().raiseError)

armanbilge · 2021-11-17T12:12:15Z

+  def continue(name: String, kernel: Kernel): Resource[F, Span[F]] =
+    make(XRaySpan.fromKernel(name, kernel, this))


I've been thinking about this, just a random (bad) idea/question:
Besides the kernel provided by the user, should this also attempt to source a kernel from the environment as a fallback? Isn't that also some form of "continuing"?

That's an interesting idea. Basically, look for the headers needed in the kernel provided by the user, and if they're missing, then check the environment? I think that could work.

Yes, I think this is worth pursuing, so that clients don't have to specialize to this backend by manually sourcing the kernel from the environment.

christiankjaer force-pushed the xray branch from e8550d7 to dd7965d Compare October 31, 2021 15:06

christiankjaer marked this pull request as ready for review October 31, 2021 15:06

This was referenced Nov 8, 2021

X-Ray enhancements christiankjaer/natchez#1

Merged

add tracing support with Natchez typelevel/feral#44

Closed

armanbilge reviewed Nov 10, 2021

View reviewed changes

tpolecat marked this pull request as draft November 10, 2021 15:20

christiankjaer force-pushed the xray branch from b513a73 to 739962c Compare November 10, 2021 15:39

Christian Kjær Larsen and others added 7 commits November 10, 2021 16:43

Added support for AWS X-Ray with an UDP socket to the X-Ray daemon

bccac0d

fix Scala 3 build errors

3581370

enable SJS support for X-Ray module

371a4f6

add XRayEnvironment for pulling X-Ray initialization info from the sy…

00cad67

…stem environment (which is useful in Lambda functions)

introduce platform-specific methods for getting information from the …

baf85db

…environment

relax cats-effect constraints on XRay types

fe4b574

Review changes

5313e7e

christiankjaer force-pushed the xray branch from 739962c to 5313e7e Compare November 10, 2021 15:44

armanbilge mentioned this pull request Nov 10, 2021

Support microsecond precision in realTime and realTimeInstant on the JVM typelevel/cats-effect#2416

Merged

Christian Kjær Larsen added 2 commits November 12, 2021 09:58

Review and fmt

4152220

Added test

2757a46

christiankjaer force-pushed the xray branch from 316b48f to 2757a46 Compare November 14, 2021 11:53

armanbilge requested changes Nov 14, 2021

View reviewed changes

armanbilge reviewed Nov 14, 2021

View reviewed changes

Final review feedback

0f7e4bf

armanbilge approved these changes Nov 14, 2021

View reviewed changes

christiankjaer marked this pull request as ready for review November 14, 2021 20:43

bpholt reviewed Nov 17, 2021

View reviewed changes

armanbilge reviewed Nov 17, 2021

View reviewed changes

tpolecat merged commit 48bf225 into typelevel:series/0.1 Nov 17, 2021

bpholt mentioned this pull request Nov 17, 2021

Iterating on X-Ray support #452

Closed

armanbilge mentioned this pull request Dec 13, 2021

MVP / 0.1.0 typelevel/feral#69

Closed

6 tasks

		def continue(name: String, kernel: Kernel): Resource[F, Span[F]] =
		make(XRaySpan.fromKernel(name, kernel, this))

Uh oh!

Uh oh!

Conversation

christiankjaer commented Sep 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpholt commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christiankjaer commented Oct 29, 2021

Uh oh!

bpholt commented Nov 9, 2021

Uh oh!

armanbilge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tpolecat commented Nov 10, 2021

Uh oh!

christiankjaer commented Nov 14, 2021

Uh oh!

armanbilge left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armanbilge Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

armanbilge left a comment

Choose a reason for hiding this comment

Uh oh!

christiankjaer commented Nov 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christiankjaer commented Nov 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpholt Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

christiankjaer commented Sep 27, 2021 •

edited

Loading

bpholt commented Oct 29, 2021 •

edited

Loading

armanbilge left a comment •

edited

Loading

armanbilge Nov 14, 2021 •

edited

Loading

christiankjaer commented Nov 14, 2021 •

edited

Loading

bpholt Nov 17, 2021 •

edited

Loading