feat(err): add context manager and tag functions#239
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added error tracking enhancements with new context management and tagging capabilities to the PostHog Python SDK.
- Introduced thread-local context management via
posthog.new_context()and a@posthog.trackeddecorator for adding metadata to exceptions - Implemented duplicate capture prevention using
__posthog_exception_capturedflag inclient.py - Warning: Thread-local storage in
scopes.pymay not be suitable for async contexts and could cause memory leaks in long-running apps - Issue: Nested contexts completely override parent tags instead of inheriting them, which may lead to lost context
- Risk: Potential circular import in
scopes.pywhen importing posthog inside the tracked decorator
6 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
This reverts commit 21ad873.
| # Grab current context tags, if any exist | ||
| context_tags = get_tags() | ||
| if context_tags: | ||
| properties.update(context_tags) |
There was a problem hiding this comment.
this placement is cool
since it looks to me like i can set a super property
and then use context to override it sometimes
that is pleasing
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
i'm invested in getting my other PR in so 🚢 🚢 🚢 🚢 🚢
(thanks for moving context to capture, i think that will super powerful)
daibhin
left a comment
daibhin
left a comment
There was a problem hiding this comment.
Epic stuff!! Think a lot of my questions & points you worked through with Paul yesterday.
Left one comment about whether tags should live on the client. I'm sure whatever we choose someone will want the alternative. Also happy to go with the approach as it is now for simplicity sake and can revisit if necessary
| @@ -0,0 +1,120 @@ | |||
| import contextvars | |||
There was a problem hiding this comment.
In our query_tagging.py we just rely on thread_local_storage = threading.local() to manage the contexts. I don't know much about the Python runtime / API but this sounds more correct to me. Do you know of any reason to use local threading or should we just not be using that in the Django app?
There was a problem hiding this comment.
We should be using contextvars I think, but since we don't use async python in our django app (as far as I know?) I think it doesn't make a huge difference.
Co-authored-by: David Newell <d.newell1@outlook.com>
| tag = tag | ||
| get_tags = get_tags | ||
| clear_tags = clear_tags | ||
| tracked = scoped |
There was a problem hiding this comment.
Did you mean for tracked to be different to scoped? It means things like @posthoganalytics.scoped in the example should actually be @posthoganalytics.tracked
There was a problem hiding this comment.
GAH
I went back and forth a few times on the name and ended up with scoped, but I guess I never fixed the export 🤦
There was a problem hiding this comment.
We can probably stamp a quick release with the change - it's breaking, but I think that's ok if we catch it fast enough
There was a problem hiding this comment.
Yeah totally fine to call it a fix. Pretty sure we're the only ones using it given it's not even a week old or documented yet
3 participants
We've been hacking around this internally with
tag_queriesfor a while, but we should just actually support it properly in the SDK.