tmc / langchaingo

LangChain for Go, the easiest way to write LLM-based programs in Go
https://tmc.github.io/langchaingo/
MIT License
3.76k stars 523 forks source link

callbacks: add openllmetry handler (WIP) #801

Closed aannirajpatel closed 3 days ago

aannirajpatel commented 2 months ago

🟡WIP for implementing an OpenLLMetry callback handler. Open questions:

  1. I have currently left OpenLLMetry client instantiation to the consumer of langchaingo, so that the OpenLLMetry callbacks handler is voided of the "client configuration" responsibility and can focus solely on implementing the callback methods. Should we instead instantiate the client within NewOpenLLMetryCallbacksHandler?
  2. The callback handler interface seems inadequate to satisfy requirements for implementing the OpenLLMetry specification for gen-ai spans, which has now been adopted into the OpenTelemetry specification: https://github.com/open-telemetry/semantic-conventions/pull/825/files#diff-72369cbec5da92c01143061b1f74d700eba3fc8f559f47fc70331dbf1bf702fe. Mainly, we are missing model information Wondering if we should:
    • (a) Update callback Handler methods to require model and other information in line with the spec linked above. OR
    • (b) Keep the callback Handler methods the same but utilize Context with predeclared keys, requiring Model implementations to pass model information. OR
    • (c) Something else (ideas welcome :)

Checklist ✅ I have read the Contributing documentation. ✅ I have read the Code of conduct documentation. ✅ I checked that there isn't already a PR that solves the problem the same way to avoid creating a duplicate. 🟡 My PR provides a description that addresses what it is solving, with a focus on integrating Mistral LLMs into LangChain. 🟡 My PR describes the source of new concepts introduced (no new concepts introduced). ✅ My PR references existing implementations as appropriate, primarily the mistral-go client library documentation. 🟡 PR should contain test coverage for new functions (TBD). 🟡 Verified that my PR passes all golangci-lint checks.

resolves #755

tmc commented 1 month ago

Thanks so much for adding this! I need to dig into it to have a stronger opinion -- have you formed one in the meantime?

aannirajpatel commented 1 month ago

Thanks so much for adding this! I need to dig into it to have a stronger opinion -- have you formed one in the meantime?

I think the ctx approach seems more apt here as it keeps the function arguments clean and will be easy to modify/extend.

recht commented 1 month ago

I was looking around to see if anybody was working on extending callback handlers with the ability to correlate start and end events, and my guess is that's also going to be needed for anything otel related. As it is now, the start and end funcs are completely independent, so when you get the end event you don't really know which start event it matches (because the callback is set on the client, which in many cases is going to be a singleton that can be used concurrently by many goroutines). So my advice would be to not rely on magic context variables, but instead create a new handler interface that includes the necessary information, and uses a chaining approach instead (like eg http filters). The interface would then be look something like

HandleLLM(ctx context.Context, req LLMRequest, next func(ctx context.Context) (LLMResponse, error)) (LLMResponse, error)
HandleChain(ctx context.Context req ChainRequest, next func(ctx context.Context) (ChainResponse, error)) (ChainResponse, error)

This also allows callbacks to implement caching, rewrite requests and responses, and other nice things.

aannirajpatel commented 1 month ago

I was looking around to see if anybody was working on extending callback handlers with the ability to correlate start and end events, and my guess is that's also going to be needed for anything otel related. As it is now, the start and end funcs are completely independent, so when you get the end event you don't really know which start event it matches (because the callback is set on the client, which in many cases is going to be a singleton that can be used concurrently by many goroutines). So my advice would be to not rely on magic context variables, but instead create a new handler interface that includes the necessary information, and uses a chaining approach instead (like eg http filters). The interface would then be look something like

HandleLLM(ctx context.Context, req LLMRequest, next func(ctx context.Context) (LLMResponse, error)) (LLMResponse, error)
HandleChain(ctx context.Context req ChainRequest, next func(ctx context.Context) (ChainResponse, error)) (ChainResponse, error)

This also allows callbacks to implement caching, rewrite requests and responses, and other nice things.

I was hoping Context's immutability would fix that - we save the span to the context with a specific key when start callback is called and pull out the span from context when end is called.

aannirajpatel commented 1 month ago

I was looking around to see if anybody was working on extending callback handlers with the ability to correlate start and end events, and my guess is that's also going to be needed for anything otel related. As it is now, the start and end funcs are completely independent, so when you get the end event you don't really know which start event it matches (because the callback is set on the client, which in many cases is going to be a singleton that can be used concurrently by many goroutines). So my advice would be to not rely on magic context variables, but instead create a new handler interface that includes the necessary information, and uses a chaining approach instead (like eg http filters). The interface would then be look something like

HandleLLM(ctx context.Context, req LLMRequest, next func(ctx context.Context) (LLMResponse, error)) (LLMResponse, error)
HandleChain(ctx context.Context req ChainRequest, next func(ctx context.Context) (ChainResponse, error)) (ChainResponse, error)

This also allows callbacks to implement caching, rewrite requests and responses, and other nice things.

That certainly sounds like a better direction. Happy to take a stab at it, unless you're time-bound.

aannirajpatel commented 2 weeks ago

I have pushed this commit for a review of the working direction as I work to fix the build: adding the OpenLLMetry module seems to not be playing nicely with existing installs of OpenTelemetry module, so we might have to change this PR to one for the introduction of OpenTelemetry handler(s) instead - the approach for the new callback methods should work just as fine for that.

aannirajpatel commented 3 days ago

Closing due to otel dependency version clash when installing OpenLLMetry - will need some additional exploration to address, opened #944 to introduce OpenTelemetry handler in the meantime.