-
Notifications
You must be signed in to change notification settings - Fork 53
Add braintrust.auto_instrument() for one-line auto-instrumentation #1265
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
base: main
Are you sure you want to change the base?
Conversation
cd05d06 to
80a1ba6
Compare
knjiang
left a comment
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.
mostly questions around maintainablility and discovery of auto-instrumentation.
code LGTM
| except ImportError as e: | ||
| logger.error(f"Failed to import Claude Agent SDK: {e}") | ||
| logger.error("claude-agent-sdk is not installed. Please install it with: pip install claude-agent-sdk") | ||
| except ImportError: |
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 dont see tests for claude_agent_sdk and others, i think we can probably add them in as well for test_auto.
i'm wnodering if there's an a way we can also enforce testing auto instrumentation?
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 know about enforcement but I added the rest!
c638c2d to
8c6d75e
Compare
manugoyal
left a comment
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 cool to me
| logger = braintrust.init_logger(project="auto-instrument-demo") | ||
|
|
||
| # Now import and use AI libraries normally - all calls are traced! | ||
| # IMPORTANT: Import AI libraries AFTER calling auto_instrument() |
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.
Is this requirement really necessary? I think module imports are shared objects,
so if you modify a module in one scope, it'll modify all imports of that module.
E.g.
In [1]: def foo():
...: import sys
...: setattr(sys, 'manuattr', 'foo')
...:
In [2]: import sys
In [3]: getattr(sys, 'manuattr')
---------------------------------------------------------------------------
AttributeError Traceback (most recent call last)
Cell In[3], line 1
----> 1 getattr(sys, 'manuattr')
AttributeError: module 'sys' has no attribute 'manuattr'
In [4]: foo()
In [5]: getattr(sys, 'manuattr')
Out[5]: 'foo'
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.
- import openai; openai.OpenAI() - works regardless of order (looks up attribute dynamically)
- from openai import OpenAI; OpenAI() - only works if imported AFTER patching (creates local binding)
py/src/braintrust/auto.py
Outdated
| try: | ||
| from braintrust.oai import patch_openai | ||
|
|
||
| patch_openai() |
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 like patch_openai swallows import errors, so if we're trying to detect
whether the patch succeeded, maybe patch_openai should return the boolean
directly?
f3cfcc9 to
d2eaa19
Compare
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
DSPy uses litellm which uses httpx. The standalone VCR in test_utils doesn't capture httpx requests (only requests/urllib3). The pytest-vcr plugin has httpx support, so span verification is done in test_dspy.py. This test now focuses on patching behavior only - verifying that auto_instrument/auto_uninstrument work correctly. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| logger.error(f"Failed to import Agno: {e}") | ||
| logger.error("Agno is not installed. Please install it with: pip install agno") | ||
| except ImportError: | ||
| # Not installed - this is expected when using auto_instrument() |
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 have the logging conditional if and only if not using auto instrumentation
| Example: | ||
| ```python | ||
| import braintrust |
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'd rather have each register itself with the library, and then have this iterate through registered instrumentations. This allows 3rd (or first) party libraries to register themselves as well (we'll need this for langchain-py and adk).
| package_dir={"": "src"}, | ||
| packages=setuptools.find_packages(where="src"), | ||
| package_data={"braintrust": ["py.typed"]}, | ||
| package_data={"braintrust": ["py.typed", "wrappers/cassettes/*.yaml"]}, |
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.
should we be including the cassettes? :O
| def _apply_openai_wrapper(client): | ||
| """Apply tracing wrapper to an OpenAI client instance in-place.""" | ||
| wrapped = wrap_openai(client) | ||
| for attr in ("chat", "responses", "embeddings", "moderations", "beta"): |
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 be using wrapt library to override class methods. i fear about the loss of types and other idiosyncratic pythonic things (signature/introspection, binding, ...) with this setattr approach
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.
yea i just didn't want to add a dependency without really making sur eit was worht itm but we can try. we did this at dd as well.
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.
| return results | ||
|
|
||
|
|
||
| def _uninstrument_openai() -> bool: |
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.
each wrapper should have this rather than in this massive auto.py file
| _internal_with_custom_background_logger, # noqa: F401 # type: ignore[reportUnusedImport] | ||
| ) | ||
| from .oai import ( | ||
| patch_openai, # noqa: F401 # type: ignore[reportUnusedImport] |
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 we should expose these
ibolmo
left a comment
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.
woops forgot to block until my questions/suggestions are reviewed

Adds a new auto_instrument() function that detects installed AI/ML libraries and automatically instruments them for tracing.
Supported integrations:
Usage: