-
Notifications
You must be signed in to change notification settings - Fork 51
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
[feat] langfuse tracing layer #498
Conversation
@ajgray-stripe as you may have seen we're porting goose over to rust. Wanted to get your thoughts on this first pass at adding in tracing and how we might adapt it to make your observability system easily pluggable |
Hey @ahau-square, thanks for the heads-up on this work! I rearranged what you've got into a couple different files (nothing too major as regards functionality, hopefully) and came up with this. (fft cherry-pick; I would have committed to your branch but I don't have the permissions to) basically, the idea is that please lmk if that makes sense! confirmed that the refactor works on my machine One thing that this would still be missing is a "plugin system" of registering observer layers without needing to explicitly enumerate them in |
// Create the span observation | ||
let mut batch = self.batch_manager.lock().await; | ||
batch.add_event( | ||
"observation-create", |
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.
probably don't need this now but i am guessing these are Langfuse specific event types. do we want Observation/Observer to be a trait and this would be LangfuseObserver?
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.
these are indeed langfuse-specific. currently the observation layer implement the layer trait, so might be confusing to have both that and an additional observe trait?
with @ajgray-stripe 's suggestion, folks can map from this schema to their own layer implementations in add_event
I think we can kick the can down the road now and revisit a refactor when someone wants to add another observability provider
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.
Looks great!
crates/goose/src/agent.rs
Outdated
@@ -5,6 +5,7 @@ use rust_decimal_macros::dec; | |||
use serde_json::json; | |||
use std::collections::HashMap; | |||
use tokio::sync::Mutex; | |||
use tracing::{debug, instrument, warn}; |
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.
will have some conflicts with this file with the mcp changes, but we can forward port easily i think
status if status.is_success() => { | ||
let response_body: LangfuseIngestionResponse = response.json().await?; | ||
|
||
for error in &response_body.errors { |
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.
kind of silly question, but what happens when you use tracing in response to a tracing integration? does it recurse? i wonder where this should go but it makes sense that we can't panic on this because the external server might be down at any given time
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 should be fine because in on_event in observation_layer.rs, we only process the event and add to the Langfuse batch if there's a current span context, but the background task that runs send_async doesn't inherit any span context
So in this case the error trace will pass through all the layers on the trace subscriber including the langfuse one, file, console, but langfuse one will not find a current span attached to it and won't process it, and the file/console layers will process it normally
9bf6b81
to
926e412
Compare
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
* v1.0: hotfix: use same executable path for dev system
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Co-authored-by: Alistair Gray <[email protected]>
Added a tracing layer that translates Rust tracing events to locally hosted Langfuse server via Langfuse API.
The layer:
Usage
Set LANGFUSE_PUBLIC_KEY and LANGFUSE_SECRET_KEY env variables for auth to your local Langfuse server or just run
just langfuse-server
The traces are also written to a log file in ~/.config/goose/logs. The name of the log for the CLI is the same as the session name the session gets saved to. TODO outside of this PR to do the same for the Goose UI.