feat: add example using Sentry V2 SDK #140
Conversation
Notes: The V1 example doesn't seem to work for me after recently trying to set up Sentry. The issue seems to be related to the `warnings` and `threading` modules that Sentry uses not being included properly into the Temporal Workflow's sandbox. This results in the Workflow execution failing to start in the worker that handles the Workflow. I don't see any issues with the Sentry interceptor on other workers that only handle activities, so it is definitely related to the Workflow sandbox. Note: I also tried disabling the worker's sandbox and this seemed to work, even though you get a sporadic error in the Temporal UI. The solution seems to be to migrate the interceptor to use V2 of Sentry's SDK, where the Hub object is now deprecated in favour of using scopes. It seems that using the scope context manager doesn't have the same issues with the warnings and threading libs (which it does still use). I've created a new example to keep the V1 example in place, since V2 only works with Python 3.6 or above.
|
|
sentry_v2/README.md
Outdated
| Note: This is a small modification of the original example [sentry (v1)](../sentry) which seems to have | ||
| issues in the Workflow interceptor, as there are modules (`warnings` and `threading`) that Sentry uses that aren't passed through | ||
| to Temporal's sandbox properly, causing the Workflows to fail to execute. |
There was a problem hiding this comment.
If Sentry V1 doesn't work anymore, let's replace the sample instead of adding a new one
There was a problem hiding this comment.
Hey @cretz - I haven't been able to test this, but I suspect now the example didn't work for me because when I installed sentry-sdk, it will have defaulted to the latest version (V2), but the example only works with V1. I imagine if you pin the version to the one in the example, it might work.
It does look like development on the V1 SDK has stopped now (last feature release was in April with a security patch in July).
So, I will let you decide if we should replace the V1 example. I'll update the PR as necessary.
There was a problem hiding this comment.
Hrmm, assuming the current sentry works, how about we just move the current one to sentry_v1 and make yours just sentry? So change the dirs and the dependency group name and the top-level README and such. Then we can come back and delete sentry_v1 later when they officially EOL it.
There was a problem hiding this comment.
@cretz I renamed sentry -> sentry_v1 and kept sentry_v2 as is, so the commit history doesn't get skewed for the original impl. Let me know if you'd prefer to make sentry_v2 -> sentry.
There was a problem hiding this comment.
Does Sentry still support v1? If it is EOL, maybe we should consider it EOL too.
sentry_v2/README.md
Outdated
| issues in the Workflow interceptor, as there are modules (`warnings` and `threading`) that Sentry uses that aren't passed through | ||
| to Temporal's sandbox properly, causing the Workflows to fail to execute. | ||
|
|
||
| > Sentry V2 only supports Python 3.6 and higher. So if you're on Python 3.5, you may need to refer to the original example |
There was a problem hiding this comment.
Temporal Python SDK only supports 3.8 or higher, so this disclaimer is not needed
There was a problem hiding this comment.
@cretz I've removed and reworded both the readme's in the sample.
Btw, you cannot have two different poetry groups containing the same package with different versions. Previously, I added a constraint on Python, so SDK v1 was installed for Python<3.6 and SDK v2 for Python>=3.6. However, since the samples project uses Python 3.10 (and the Temporal SDK only supports 3.8 and higher), I think this hack would confuse users, as it would always install SDK v2 and the original sample wouldn't work as expected.
I've updated the V1 README to require the user to install sentry-sdk==1.11.0 with pip rather than using poetry.
There was a problem hiding this comment.
you cannot have two different poetry groups containing the same package with different versions
Ug, that is unfortunate. I do still think we should have the Poetry groups named after the sample they represent, even if it's a dupe for sentry_v1 and sentry_v2 (or there isn't even a sentry_v1 group at all and pip install is manual). This also may give more credence to the approach of making sentry_v2 just sentry.
There was a problem hiding this comment.
I've asked on the Sentry's discussion board what the timeline is for v1 support. Will get back once they provide an answer.
One idea would be to just replace the v1 impl. with the new v2 integration, keep the example called sentry, and then add a link in the README to the original v1 commit for posterity?
There was a problem hiding this comment.
Sure, that would work well. Either way I think we want this to be the primary sentry sample since we have to have the dependency group named that.
- Renamed sentry sample to sentry_v1 until it reaches EOL - Clarified readme for both samples
|
Uh oh, v3 so soon? Ok, I guess we will cross that bridge when we get there. Otherwise, I think this looks good and I'll merge if/when CI passes. The purpose is to serve as a sample, so users can adapt it as they need for their version of tooling. |
|
Hey @cretz, do you want me to rebase the PR? |
What was changed
Added a new example to set up Sentry using the V2 SDK.
Why?
The original Sentry integration only works with Sentry SDK v1, due to issues with the
warningsandthreadingmodules that Sentry uses not being appropriately included in the Temporal Workflow's sandbox when SDK v2 is installed. (Presumably, Sentry SDK v2 made some internal changes, so these modules are now causing issues.)Given that v1 is now deprecated and that v2 will be installed by default if the user runs
poetry add sentry-sdkin their own project, this new sample provides the required integration for Sentry SDK v2 to work without sandbox issues.The necessary changes only required migrating the use of the
Hubobject (which is now deprecated) to using Sentry scopes.The original Sentry v1 sample is kept as the SDK still receives security patches and is not yet EOL.
Checklist
https://temporalio.slack.com/archives/CTT84RS0P/p1725394300914329
Tested manually in my application using Sentry.io
Added a new example with a README explaining the reasons/difference between the original example and the v2 example.