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.
|
|
- 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? |
|
@gregbrowndev - I just merged main into your branch, no problem. Looks like just the CLA needs to be signed (see link above pointing to https://cla-assistant.io/temporalio/samples-python?pullRequest=140). May need to make a comment here when done so I can get notified to merge this. |
|
Hello, I just tested with If I don't do it I get |
|
Sorry @cretz, I've only just seen that this PR was blocked by me not signing the CLA. I've done so now. Could you comment on @M0NsTeRRR 's message above? Is the with workflow.unsafe.imports_passed_through():
import sentry_sdk
from sentry.interceptor import SentryInterceptor |
|
After reading it again, I realized that I didn’t import the interceptor in my workflow, which is likely why I got this error — my bad. However, I think it makes more sense for the example to whitelist it at the worker level, since every exception will be caught by the Sentry interceptor. Thanks for your answer @gregbrowndev :) |
Thanks! Would you be willing to merge
Yes, they are the same thing, but note some imports are done lazily by some libraries so we have to account for that too. What we usually do in newer samples and what we encourage users to do (and would like to here, though don't have to now), is to have workflows be in their own file instead of a shared file. |
|
I have the same
Edit: The answer is that you should do it where you import your workflows to build the list of workflows and activities. |
- Update sentry version in uv - Test code review suggestions. I'm finding that the integration is failing and not sending events to my Sentry account
- Having gevent installed causes the workflow interceptor to break - gevent is installed by default as its used in one of the other samples and it is included in uv's "default-groups". I've updated the sentry README.md for how to prevent it being installed
- After splitting the worker, workflow, and activity into their own
modules, I started to see workflow task error:
TypeError: metaclass conflict: the metaclass of a derived class
must be a (non-strict) subclass of the metaclasses of all its bases
- Fix the above error by explicitly passing through "sentry_sdk" to
the workflow sandbox in the worker options
|
@cretz ready to review again, sorry for dragging on this. The Notes:
|
Is it possible to add some test that doesn't require Sentry for this sample? Our Sentry V1 sample started breaking and we didn't know because there were no tests. Also, when the test is added, we can easily replicate this error and I can help debug it.
Yeah, we have an issue to remove default groups, they were added by accident: #190
The worker just uses this inside its function to interrupt the |
Yes, good idea I will add these tests. I should get a bit of time this weekend
Sounds good 👍🏻
It's not that the exception bubbles out of the worker, it's just that the |
| workflow_runner=SandboxedWorkflowRunner( | ||
| restrictions=SandboxRestrictions.default.with_passthrough_modules( | ||
| "sentry_sdk" | ||
| ) | ||
| ), |
There was a problem hiding this comment.
@cretz: I've added tests around the Sentry interceptor. All passing for me.
Btw, if you comment out the workflow_runner here so sentry_sdk isn't passed through to the worker, and run the test without the gevent library installed:
uv sync --no-default-groups --dev --group sentry
uv run --no-default-groups --dev --group sentry pytest ./tests/sentryYou can see the metaclass conflict error:
File "/Users/greg/Development/samples-python/.venv/lib/python3.13/site-packages/temporalio/worker/workflow_sandbox/_importer.py", line 234, in _import
mod = importlib.__import__(name, globals, locals, fromlist, level)
File "<frozen importlib._bootstrap>", line 1486, in __import__
File "<frozen importlib._bootstrap>", line 1415, in _handle_fromlist
File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 1026, in exec_module
File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
File "/Users/greg/Development/samples-python/.venv/lib/python3.13/site-packages/urllib3/exceptions.py", line 261, in <module>
class IncompleteRead(HTTPError, httplib_IncompleteRead):
...<18 lines>...
)
TypeError: metaclass conflict: the metaclass of a derived class must be a (non-strict) subclass of the metaclasses of all its bases
So looks like the having the imports_passed_through in the workflow / interceptor isn't enough.
Also, if you run the tests with all groups synced, it still fails but now with a gevent related error:
uv sync
uv run pytest ./tests/sentryError:
File "/Users/greg/Development/samples-python/.venv/lib/python3.13/site-packages/temporalio/worker/workflow_sandbox/_importer.py", line 234, in _import
mod = importlib.__import__(name, globals, locals, fromlist, level)
File "<frozen importlib._bootstrap>", line 1466, in __import__
File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
File "<frozen importlib._bootstrap>", line 1331, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 935, in _load_unlocked
File "<frozen importlib._bootstrap_external>", line 1026, in exec_module
File "<frozen importlib._bootstrap>", line 488, in _call_with_frames_removed
File "/Users/greg/Development/samples-python/.venv/lib/python3.13/site-packages/gevent/os.py", line 539, in <module>
__imports__ = copy_globals(os, globals(),
names_to_ignore=__implements__ + __extensions__,
dunder_names_to_keep=())
File "/Users/greg/Development/samples-python/.venv/lib/python3.13/site-packages/gevent/_util.py", line 90, in copy_globals
items = iteritems(source.__dict__)
TypeError: descriptor 'items' for 'dict' objects doesn't apply to a '_RestrictedProxy' object
However, if you run the tests with all extras synced and with the workflow_runner set on the worker, then the tests pass. Seems explicitly setting this on the worker fixes both the metaclass issue and the gevent issue 👍🏻
|
@cretz one last thing to mention. I've been working on a fix for the I noticed it was easy to break the Sentry interceptor's reflection features, e.g. Would you consider making the Sentry interceptor a native integration of the Python SDK? I'd be happy to open a PR to the main SDK repo! I'm also working on a Sentry interceptor for the TypeScript SDK too which doesn't have any user-provided samples yet. |
| capture_exception() | ||
| scope.set_context("temporal.activity.input", asdict(arg)) | ||
| scope.set_context("temporal.activity.info", activity.info().__dict__) | ||
| scope.capture_exception() |
There was a problem hiding this comment.
It didn't capture anything for me until I've changed this from scope.capture_exception() to sentry_sdk.capture_exception()
Same for workflow
There was a problem hiding this comment.
@kenzmed, did you see this in your own app?
The code sample works for me with a live Sentry backend:
If you're running it in your own app, it's probably because you have Sentry SDK v1 installed. This sample is for Sentry v2. Take a look at the original interceptor.
Keep in mind, the original interceptor doesn't work for Sentry SDK v2 and SDK v1 is EOL.
👍 Approved CI to run. This may benefit from #212.
Not sure this is something we can officially maintain and support outside of a sample at this time. Ideally the sample can just be referenced by users. The tests in the sample will ensure it does not break so it can be sure to continue to work. |
Making Sentry a first class citizen of your SDK would be great I believe for the Python's adoption. |
|
@gregbrowndev - may need a main merge and a regen the lock file |
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.