Remove end-on-exit stuff #676
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #676 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 137 137
Lines 10750 10721 -29
Branches 1473 1470 -3
=========================================
- Hits 10750 10721 -29 ☔ View full report in Codecov by Sentry. |
Deploying logfire-docs with
|
| Latest commit: |
9cdea14
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a2256a13.logfire-docs.pages.dev |
| Branch Preview URL: | https://dmontagu-remove-end-on-exit.logfire-docs.pages.dev |
| @@ -1898,8 +1896,7 @@ def __getattr__(self, name: str) -> Any: | |||
|
|
|||
| def __enter__(self) -> LogfireSpan: | |||
| with handle_internal_errors(): | |||
| self.end_on_exit = True | |||
| if self._span is None: | |||
| if self._span is None: # pragma: no branch | |||
There was a problem hiding this comment.
I think this line could be removed (and the no branch dropped), and just unconditionally set self._span. Wasn't sure if it was worth doing that too though at this time, given this code is likely to be refactored soon anyway.
I mostly wanted to open this PR now because of the chance that I was misunderstanding the importance of this API and it needed to be accounted for in some way while attempting to refactor. But the more I think about it the more I think it's only purpose (at least today) is a syntax sugar thing that's separate from the actual behavior
Digging through the history of the code, it looks like
end_on_exitwas useful when it was first introduced, but the code has been refactored to make this no longer used in any way in our existing code.Arguably it's part of the public API, but I don't see any associated docs, and it's not clear that it was intended to be used that way. I'm happy to keep it if we think it serves a useful purpose but while looking into how we can refactor things to make it so the LogfireSpan is created by the tracer, this struck me as possibly-no-longer-worth-keeping.
If, even after refactoring, we want to keep the ability to use the contextmanager protocol to open a span but keep it open after the contextmanager is exited, it feels like it doesn't need to be dynamic and could be an argument to the contextmanager function. That said, I also feel like even that shouldn't be necessary, and we could add another (non-contextmanager) method that starts the span, or just use
__enter__directly.