-
Notifications
You must be signed in to change notification settings - Fork 49
fix: change scoped export, add capturing param #257
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
Conversation
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.
PR Summary
Updates scoping functionality in PostHog Python SDK to make error tracking optional while retaining tagging infrastructure.
- Added new
capturingparameter tonew_contextandscopedfunctions, defaulting to True for backward compatibility - Renamed exported decorator from
trackedtoscopedinposthog/__init__.pyfor better clarity - Incremented version from 4.7.0 to 4.8.0 to reflect new functionality
- Maintained tag isolation and context inheritance behavior while making error tracking optional
4 files reviewed, no comments
Edit PR Review Bot Settings | Greptile
posthog/scopes.py
Outdated
| yield | ||
| except Exception as e: | ||
| capture_exception(e) | ||
| if capturing: |
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 could default to enable_exception_autocapture if this is not set
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.
Huh, for some reason I thought that was client specific?
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.
It can be globally set using the proxy if the customer has configured their setup like
import posthog
posthog.enable_exception_autocapture = True
If that is None we should default (probably to 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.
Defaults to False rather than None, sadly :/.
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, given if we deferred to the global setting it'd always be off, it's good enough for now to default to on and let users disable. We can revisit it later, if we decide it's warranted
More broadly, I dislike complex behaviours and global configurations taking precedence over local ones - there may be good reasons a user wants to say "absolutely do not autocapture exceptions at this scope level", and we should respect it, imo.
Figured while I was here we may as well let people use the tagging infrastructure without forcing them to use error tracking - it seems like a nice pattern for using posthog generally.
I do think we should default opt-in to error tracking tho - if we get complaints, we can expose a global config to disable it at the client level, but capturing exceptions in the most deeply nested context means getting the most tags on them, which is the ideal scenario.