[chore] Add warnings when instrumentation is used before initialization#3177
[chore] Add warnings when instrumentation is used before initialization#3177astandrik wants to merge 3 commits intoAgenta-AI:mainfrom
Conversation
…ed before ag.init()
dosubot
bot
added
the
size:L
|
@astandrik is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
|
|
There was a problem hiding this comment.
Pull request overview
This PR implements a warning mechanism to alert users when instrumented functions are called before ag.init() is invoked. The implementation adds pre-initialization checks across all four function types (sync, async, sync generator, async generator) that emit a one-time RuntimeWarning to guide users toward correct SDK initialization.
Key Changes
- Added
_is_tracing_initialized()helper to check if the Agenta SDK tracing is properly initialized - Implemented
_warn_instrumentation_before_init_once()that emits a one-time warning with clear guidance when tracing is not initialized - Integrated early-return logic into all four instrumentation wrappers to gracefully handle pre-init calls
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sdk/agenta/sdk/decorators/tracing.py | Adds initialization checking logic, warning mechanism, and early-return behavior for all four function types when tracing is not initialized |
| sdk/tests/unit/test_tracing_decorators.py | Adds test class with 3 tests covering sync functions, async functions, and sync generators to verify warning behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def test_async_generator_warns_once_and_still_executes(self, recwarn): | ||
| @instrument() | ||
| async def gen(): | ||
| yield "a" | ||
| await asyncio.sleep(0.001) | ||
| yield "b" | ||
|
|
||
| first = [] | ||
| async for item in gen(): | ||
| first.append(item) | ||
| assert first == ["a", "b"] | ||
|
|
||
| second = [] | ||
| async for item in gen(): | ||
| second.append(item) | ||
| assert second == ["a", "b"] | ||
|
|
||
| runtime_warnings = [ | ||
| w for w in recwarn if issubclass(w.category, RuntimeWarning) | ||
| ] | ||
| assert len(runtime_warnings) == 1 |
There was a problem hiding this comment.
The test validates the number of warnings but doesn't check the warning message content like the sync function test does (lines 711-713). Consider adding similar assertions to verify the warning message contains "called before ag.init()" and "Fix: call ag.init()" for consistency and completeness.
| is_sync_generator = isgeneratorfunction(handler) | ||
| is_async_generator = isasyncgenfunction(handler) | ||
|
|
||
| handler_name = f"{getattr(handler, '__module__', '<unknown>')}.{getattr(handler, '__qualname__', getattr(handler, '__name__', '<unknown>'))}" |
There was a problem hiding this comment.
This line exceeds typical line length conventions (appears to be ~140+ characters). Consider breaking it into multiple lines for better readability. For example, compute qualname separately first, then construct the full handler_name.
| handler_name = f"{getattr(handler, '__module__', '<unknown>')}.{getattr(handler, '__qualname__', getattr(handler, '__name__', '<unknown>'))}" | |
| qualname = getattr(handler, '__qualname__', getattr(handler, '__name__', '<unknown>')) | |
| handler_name = f"{getattr(handler, '__module__', '<unknown>')}.{qualname}" |
|
|
||
| log = get_module_logger(__name__) | ||
|
|
||
| _PREINIT_INSTRUMENTATION_WARNING_EMITTED = False |
There was a problem hiding this comment.
The global variable _PREINIT_INSTRUMENTATION_WARNING_EMITTED is not thread-safe. In a multi-threaded environment, multiple threads could simultaneously check if the warning has been emitted (line 56), both see False, and both proceed to emit the warning. This could result in multiple warnings being emitted instead of just one. Consider using a threading lock or thread-local storage to ensure thread-safety.
| async def test_async_function_warns_once_and_still_executes(self, recwarn): | ||
| @instrument() | ||
| async def mul(x, y): | ||
| await asyncio.sleep(0.001) | ||
| return x * y | ||
|
|
||
| assert await mul(2, 3) == 6 | ||
| assert await mul(3, 4) == 12 | ||
|
|
||
| runtime_warnings = [ | ||
| w for w in recwarn if issubclass(w.category, RuntimeWarning) | ||
| ] | ||
| assert len(runtime_warnings) == 1 |
There was a problem hiding this comment.
The test validates the number of warnings but doesn't check the warning message content like the sync function test does (lines 711-713). Consider adding similar assertions to verify the warning message contains "called before ag.init()" and "Fix: call ag.init()" for consistency and completeness.
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Thanks @astandrik for the PR!
I have tested it and it works fine.
It feels to me that the solution is a bit too verbose, however I don't have a solution to make it simpler.
I asked @junaway for a review, he knows that part of the code best and he can judge.
junaway
changed the title
3 participants
Closes #3171