-
Notifications
You must be signed in to change notification settings - Fork 168
Warn on Using Span Inside Generator Functions #1430
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?
Changes from all commits
d109be0
16488d2
0bf9ca9
d5b5095
d57fc44
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,6 +186,7 @@ def _span( | |
_span_name: str | None = None, | ||
_level: LevelName | int | None = None, | ||
_links: Sequence[tuple[SpanContext, otel_types.Attributes]] = (), | ||
_warn_if_inside_generator: bool = True, | ||
) -> LogfireSpan: | ||
try: | ||
if _level is not None: | ||
|
@@ -196,6 +197,35 @@ def _span( | |
else: | ||
level_attributes = None | ||
|
||
# we go two levels back to find the caller frame, as this method is called by logfire.span() method | ||
caller_frame = inspect.currentframe().f_back.f_back # type: ignore | ||
|
||
# check if the caller is a generator function by checking the co_flags attribute of the code object | ||
# and doing bit-wise AND checking for CO_GENERATOR or CO_ASYNC_GENERATOR value match | ||
caller_is_generator = bool( | ||
caller_frame and caller_frame.f_code.co_flags & (inspect.CO_GENERATOR | inspect.CO_ASYNC_GENERATOR) | ||
) | ||
|
||
is_from_context_manager = False | ||
|
||
# Check if this call is coming from inside a context manager generator by inspecting call stack frames | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this logic is generally correct. But where is it being tested anyway? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here's an example of this missing bad usage: import logfire
logfire.configure()
class MyContextManager:
def __enter__(self):
self.span = logfire.span('MyContextManager') # correct usage
items = bad()
for _ in items:
break
logfire.info('after bad') # incorrectly appears under 'bad'
self.span.__enter__()
return self
def __exit__(self, exc_type, exc_value, traceback):
self.span.__exit__(exc_type, exc_value, traceback)
return True
def bad():
with logfire.span('bad'): # incorrect usage
for x in range(3):
yield x
with MyContextManager():
logfire.info('Inside context manager') # incorrectly does not appear under 'MyContextManager' I suggest leaving this for now, it's OK for the user to get some redundant warnings. But it might be possible to follow up with improvement detecting the contextlib decorators. |
||
if caller_is_generator: | ||
previous_frame, origin_frame = inspect.currentframe().f_back, caller_frame.f_back # type: ignore | ||
|
||
for frame in [previous_frame, caller_frame, origin_frame]: | ||
code_name = frame.f_code.co_name # type: ignore | ||
if code_name in ['__enter__', '__aenter__']: | ||
is_from_context_manager = True | ||
break | ||
|
||
# usage within the context manager lifespan is legitimate, since the context manager controls the span's | ||
# lifespan, and will ensure proper separation of concerns | ||
if caller_is_generator and _warn_if_inside_generator and not is_from_context_manager: | ||
warnings.warn( | ||
'Span is inside a generator function. See https://logfire.pydantic.dev/docs/reference/advanced/generators/#move-the-span-outside-the-generator.', | ||
RuntimeWarning, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should be a UserWarning |
||
) | ||
|
||
stack_info = get_user_stack_info() | ||
merged_attributes = {**stack_info, **attributes} | ||
|
||
|
@@ -538,6 +568,7 @@ def span( | |
_span_name: str | None = None, | ||
_level: LevelName | None = None, | ||
_links: Sequence[tuple[SpanContext, otel_types.Attributes]] = (), | ||
_warn_if_inside_generator: bool = True, | ||
**attributes: Any, | ||
) -> LogfireSpan: | ||
"""Context manager for creating a span. | ||
|
@@ -557,18 +588,21 @@ def span( | |
_tags: An optional sequence of tags to include in the span. | ||
_level: An optional log level name. | ||
_links: An optional sequence of links to other spans. Each link is a tuple of a span context and attributes. | ||
_warn_if_inside_generator: Set to `False` to prevent a warning when instrumenting a generator function. | ||
attributes: The arguments to include in the span and format the message template with. | ||
Attributes starting with an underscore are not allowed. | ||
""" | ||
if any(k.startswith('_') for k in attributes): | ||
raise ValueError('Attribute keys cannot start with an underscore.') | ||
|
||
return self._span( | ||
msg_template, | ||
attributes, | ||
_tags=_tags, | ||
_span_name=_span_name, | ||
_level=_level, | ||
_links=_links, | ||
_warn_if_inside_generator=_warn_if_inside_generator, | ||
) | ||
|
||
@overload | ||
|
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.
to match logfire.instrument