-
-
Notifications
You must be signed in to change notification settings - Fork 693
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
Tracing: Added Langsmith Support #1069
base: main
Are you sure you want to change the base?
Conversation
@@ -13,7 +13,6 @@ import ( | |||
//nolint:all | |||
type Handler interface { | |||
HandleText(ctx context.Context, text string) | |||
HandleLLMStart(ctx context.Context, prompts []string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This call was dead code, it was not called anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of good work here! Can you also add a doc.go file to the langsmith package and write comments for exported symbols?
langsmith/tracer.go
Outdated
|
||
var _ callbacks.Handler = (*LangChainTracer)(nil) | ||
|
||
type LangChainTracer struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe LangChain is a little redundant here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep make sense.
callbacks/context.go
Outdated
// nolint: gochecknoglobals | ||
var callbackHandlerKey = contextKeyType(0) | ||
|
||
func CallbackHandler(ctx context.Context) Handler { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function name is a somewhat redundant and vague. Maybe something like GetHandlerFromContext instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
callbacks/context.go
Outdated
return nil | ||
} | ||
|
||
func WithCallback(ctx context.Context, handler Handler) context.Context { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think a name like this works outside of the context package: callbacks.WithCallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about SetHandlerInContext
@@ -42,21 +44,22 @@ func Call(ctx context.Context, c Chain, inputValues map[string]any, options ...C | |||
fullValues[key] = value | |||
} | |||
|
|||
callbacksHandler := getChainCallbackHandler(c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also check if there is a callback handler in the chain
I pushed the requested changes, add a |
This PR introduces LangSmith tracing to the langchaingo library, enhancing debugging and monitoring capabilities for applications leveraging large language models (LLMs). These changes align with our observability needs and have been successfully running in production for several months.
We have subsequent PR ready to improve Inputs and Outputs capturing streamingfast#4
Key Features
LangSmith Tracing Integration:
callbacks.Handler
to enable tracing capabilities.Refactor and Enhancements:
chains.Call
.chain.Call
was not passed to thellm.Model
instance.Backward Compatibility
No breaking changes have been introduced. Tracing is optional and does not affect existing functionality when disabled.
Future Improvements
Refactor HandleLLMGenerateContentStart and HandleLLMGenerateContentEnd into a centralized GenerateContent function in the llms package to reduce code duplication across llms models.
PR Checklist
memory: add interfaces for X, Y
orutil: add whizzbang helpers
).Fixes #123
).golangci-lint
checks.