-
Notifications
You must be signed in to change notification settings - Fork 31
fix(llm-observability): parallel traces #172
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
Conversation
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.
A possible code clarity suggestion, but overall looking good
posthog/ai/langchain/callbacks.py
Outdated
def _capture_trace(self, run_id: UUID, *, outputs: Optional[Dict[str, Any]]): | ||
trace_id = self._get_trace_id(run_id) | ||
run = self._pop_run_metadata(run_id) |
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.
Pretty sneaky to pop in a _capture_...
method, how about renaming it to _pop_trace_and_capture
for clarity? Alternatively, we can just do self._runs[run_id]
here and pop outside of here
input: Any | ||
"""Input of the run: messages, prompt variables, etc.""" | ||
name: str | ||
"""Name of the run: chain name, model name, etc.""" | ||
provider: str | ||
"""Provider of the run: OpenAI, Anthropic""" | ||
model: str | ||
"""Model used in the run""" | ||
model_params: Dict[str, Any] | ||
"""Model parameters of the run: temperature, max_tokens, etc.""" | ||
base_url: str | ||
"""Base URL of the provider's API used in the run.""" | ||
start_time: float | ||
"""Start time of the run.""" | ||
end_time: float | ||
"""End time of the run.""" |
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.
Was also thinking of traces as runs in my earlier PR, but then thought Run
is maybe a bit too generation specific for the trace use case (e.g. traces don't have a model). So possibly we could have a GenerationMetadata
dataclass, a TraceMetadata
one, and later SpanMetadata
for clearer separation.
But for now RunMetadata
for both is indeed a better model than the simplistic one I went with before ✅
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.
Yeah, it makes sense. I'll do that in the spans PR.
Problem
Traces implementation doesn't catch parallel traces if a single instance of LangChain callbacks is used.
Changes
Store multiple parallel traces in the trace storage. Also, I've added capturing of trace latency.