Skip to content

add tracing support with Natchez#44

Closed
bpholt wants to merge 5 commits into
typelevel:mainfrom
Dwolla:tracing
Closed

add tracing support with Natchez#44
bpholt wants to merge 5 commits into
typelevel:mainfrom
Dwolla:tracing

Conversation

@bpholt

@bpholt bpholt commented Nov 8, 2021

Copy link
Copy Markdown
Member

I've been using this with a locally published version of christiankjaer/natchez#1 and typelevel/natchez#427 and it seems to work.

My thought is that we can bake in no-op tracing, and then the user can mix in a trait that implements tracing for their preferred backend. It doesn't compile here yet (because of the missing artifacts) but the XRayTracing trait is an example of how that could work. WDYT?

@bpholt bpholt self-assigned this Nov 8, 2021
context: Context,
routes: HttpRoutes[IO]): IO[Some[ApiGatewayProxyStructuredResultV2]] =
routes: HttpRoutes[IO])(
implicit T: Trace[IO]): IO[Some[ApiGatewayProxyStructuredResultV2]] =

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How does this Trace propagate to userland code? Apologies if I'm missing something.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, good catch. I've only made changes to IOLambda and the IOLambdaPlatforms to make sure it worked at that level. We'd need to add the implicit Trace to the other abstract methods if we want to do this consistently.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Aha, I thought it might be using some IOLocal magic :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I need to think a little more on how the ApiGatewayProxyHttp4sLambda fits in here. (Open to suggestions if you see a way to do it with the current structure.)

We may need to change routes to be def routes: Resource[IO, HttpRoutes[Kleisli[IO, Span[IO], *]]]? But even that doesn't fit perfectly if all we have here is the implicit Trace[IO]. Not sure yet.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I kind of punted and added the Natchez server middleware to ApiGatewayProxyHttp4sLambda. The main problem I see is that doesn't make the Trace[IO] capability available to anything defined in def routes: Resource[IO, HttpRoutes[IO]], meaning I'm not sure how application code could open its own spans or propagate the trace onwards if it makes calls to other services.

@armanbilge it looked like you started a new branch that (from the commit messages) might be reworking the structure a little bit to help with this. I'm going to pause here to see what you come up with. 🙂

@armanbilge armanbilge left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is cool! Still gotta wrap my head around all the pieces 😅

Comment thread lambda/js/src/main/scala/feral/lambda/IOLambdaPlatform.scala Outdated
}

override protected def traceRootSpan(name: String): Resource[IO, Span[IO]] =
Resource.eval(XRayEnvironment[IO].kernelFromEnvironment).flatMap { kernel =>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This strategy for extracting the kernel might only apply to normal AWS Lambdas—e.g. for ApiGatewayProxyHttp4sLambda the kernel should probably be extracted from the incoming HTTP request headers, not the Lambda's environment. 🤔

@bpholt

bpholt commented Nov 19, 2021

Copy link
Copy Markdown
Member Author

I'm going to close this PR in favor of one of the other open tracing PRs—it has served its purpose 🙂

@bpholt bpholt closed this Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants