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.

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

[WIP] TracedIOLambda #240

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

skirsdeda
Copy link
Contributor

This solves #233, I think 🙂

As I found out, IOSetup.setup needs to get context of the first request to be able to construct Trace[IO] resource. And then, all custom resources are memoized separately so that any trace spans produced during init are part of main request span. This should be evident from TracedIOLambdaSuite.

This turned out to be quite a lot of code 😓 I am sure this is not the best implementation possible because my FP skills were at the limit 😄 It would probably be nicer if IOLambda and TracedIOLambda shared more code. Also, there should be TracedIOLambda.Simple to match IOLambda.Simple.

Leaving as work-in-progress and waiting for feedback.

@armanbilge
Copy link
Member

Thank you very much for opening this!

Spans produced during initialization would get recorded as children of root span for the first ever request to that lambda instance.

I haven't looked in detail at the code yet, but I guess implementing this semantic is what required such extensive changes? It seems tricky to me :) I'm a little curious what the motivation is.

Also it seems like this would prevent the Lambda from being able to initialize pro-actively prior to receiving its first request, but maybe this does not matter?

@skirsdeda
Copy link
Contributor Author

The motivation is quite simple: make it possible to useskunk with all it's tracing capabilities.
The way skunk works is as follows:

  1. DB session is created as Resource[IO, Session[IO]] and it takes Trace[IO] as an implicit param. On acquisition of this resource, DB connection pool is initialized and this results in a "pool" span. Obviously, when using in feral, we want this memoized and only happen on the first request. This part would be equivalent to "init" span in TracedIOLambdaSuite.
  2. When DB statements are executed, skunk Session produces "query" spans. These should appear in all requests, which means that Trace[IO] instance should be a single one and be aware of changing context of subsequent requests. Again, equivalent of "query" span in TracedIOLambdaSuite.

As I described in #233, with existing tracing capabilities in feral this just doesn't work. All spans produced by skunk Session get "lost".

@armanbilge
Copy link
Member

Right, thanks. The part that I'm less sure about is why the traces during the initialization should belong specifically to the span of the initial request, instead of a root "initialization" span for the Lambda itself. IIUC, it would mean that the Lambda is unable to initialize until the first request arrives, which may not be desirable.

@skirsdeda
Copy link
Contributor Author

@armanbilge you are right. When using provisioned concurrency, lambda could get initialized before first request. So I overdid this a little bit 😸 I'll try to change this to have "init" span for initialization. Thanks for prompt response!

@armanbilge
Copy link
Member

No problem, this is tricky stuff and I appreciate your work here! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants