-
Notifications
You must be signed in to change notification settings - Fork 167
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?
Conversation
0a17875
to
e52430e
Compare
e52430e
to
16488d2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
@alexmojaki please review this code when convenient |
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.
Please also add to the docs
_span_name: str | None = None, | ||
_level: LevelName | int | None = None, | ||
_links: Sequence[tuple[SpanContext, otel_types.Attributes]] = (), | ||
_warn_if_inside_generator: bool = True, |
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.
_warn_if_inside_generator: bool = True, | |
_allow_generator: bool = False, |
to match logfire.instrument
|
||
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 comment
The 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 comment
The 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 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be a UserWarning
Addresses the concerns mentioned in #1424, ensuring the span calls raise runtime warnings with apt documentation link, when used inside generator functions.
Test script output: