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

feat: add observability plugin system #227

Merged
merged 11 commits into from
Nov 13, 2024

Conversation

ajgray-stripe
Copy link
Contributor

Description

See #203 for background; summary of that issue is that we'd like to have a somewhat more abstracted plugin system for observability/tracing so that we can have our own house-built telemetry plugin. I ultimately opted for the relatively lightweight option of just copying Langfuse's API for this PR; I wasn't confident enough in a refactor that was any more different.

In particular, the substantive changes made here are:

  • Creates an "observer" plugin system modeled off the existing plugin systems in this repo
  • That includes talking with profiles.yaml, pyproject-based entrypoint discovery for plugins, etc.
  • Pulling out the langfuse's integration observe_wrapper decorator into a method that calls all registered observers
  • Mostly moves current langfuse_wrapper.py code into observers/langfuse.py; I didn't really touch the code in there much. Also makes that plugin a default
    Review commit-by-commit for the logical flow here.

Testing

  • I wrote a few unit tests. I checked that both goosewide and exchange tests all passed locally.

I also did some manual testing, with a couple minor hacks to get around not actually being able to test fully end-to-end with Langfuse (the environment I'm using didn't make that easy+fast so I just did this instead).

  • With the following temporary patches on top of this branch:
    image
    disabling actually authenticating Langfuse

image
and instead just logging to a file for visibility

, as well as this profile:

image

we can see that Goose runs correctly, with startup messages showing that it's correctly (trying to) initialize the Langfuse client:

image

and we correctly log method calls.

image

  • Without the --tracing flag, goose runs without errors as expected:

image

@michaelneale
Copy link
Collaborator

nice - looks good to me cc @ahau-square

@ahau-square
Copy link
Collaborator

I'm getting some different behavior in this for the langfuse tracing - seems like my traces are no longer registering to the same session. Have to dig in more to see why

@lifeizhou-ap
Copy link
Collaborator

lifeizhou-ap commented Nov 12, 2024

Hey @ajgray-stripe

Nice work! Codes are clean and easy to read. Thank you!

I had same issue as @ahau-square and saw “No trace found in the current context” message when Langfuse tracing is enabled. I had a look, it seems to be related to this code

@wraps(fn)
                def wrapped_fn(*fargs, **fkwargs) -> Callable:  # noqa: ANN002, ANN003
                    # group all traces under the same session
                    if fn.__name__ == "reply":
                        langfuse_context.update_current_trace(session_id=fargs[0].name)

                    return langfuse_context.observe(*args, **kwargs)(fn)(*fargs, **fkwargs)

                return wrapped_fn

It seems that the langfuse_context.update_current_trace needs to be called within the observe context. Doc

I tried to fix it and created a temp draft PR with the fix and it seems to work #238.

@ajgray-stripe
Copy link
Contributor Author

Thanks for the patches, @lifeizhou-ap -- incorporated those here! (and managed to get langfuse running locally, so confirmed that I see traces there with those changes)

Copy link
Collaborator

@ahau-square ahau-square left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the fixes for the langfuse integration too, tried it out and seems like the behavior now matches the previous implementation. this lgtm! excited for this!

@ahau-square ahau-square merged commit d30b524 into block:main Nov 13, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants