Skip to content
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

Sentry interceptor example #23

Merged
merged 13 commits into from
Dec 2, 2022
Merged

Conversation

nediamond
Copy link
Contributor

What was changed

added Sentry interceptor example based on discussion in slack thread

Why?

Sentry is a popular error monitoring tool & interceptors aren't super intuitive to make from scratch, hopefully this can be a good starting point for folks

Checklist

  1. How was this tested:
    Executed locally

  2. Any docs updates needed?
    not sure

@CLAassistant
Copy link

CLAassistant commented Oct 26, 2022

CLA assistant check
All committers have signed the CLA.

@nediamond nediamond marked this pull request as draft October 26, 2022 23:30
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a couple of comments. Looks great!

Some other that I think would help:

  • Add a README.md in this folder w/ details on how to run (see how OTel sample does it in OpenTelemetry sample #18)
  • Add a "dependency group" for sentry in pyproject.toml to add the sentry_sdk dependency (see how OTel sample does it in OpenTelemetry sample #18)
  • Separate the starter from the worker. We are doing this in all of the non-hello samples, and we do it in all SDKs, to emphasize that workers are often completely separate from starters (see how OTel sample does it in OpenTelemetry sample #18)

If the Sentry SDK comes with utilities to capture/assert output in a test, a test in tests/sentry would be great. But not required or anything.

sentry/interceptor.py Outdated Show resolved Hide resolved
sentry/interceptor.py Outdated Show resolved Hide resolved
sentry/interceptor.py Show resolved Hide resolved
sentry/interceptor.py Outdated Show resolved Hide resolved
sentry/worker.py Outdated Show resolved Hide resolved
sentry/worker.py Outdated Show resolved Hide resolved
@nediamond
Copy link
Contributor Author

will try to implement the tests/readme/worker changes this weekend

@cretz
Copy link
Member

cretz commented Nov 15, 2022

@nediamond - Wanted to follow up here. I think this is a great sample. Let me know if there's anything I can do to help move it along.

@nediamond
Copy link
Contributor Author

@cretz sorry got busy with life stuff will set some time aside for this before next week - feel clear on what needs to be done thanks

@nediamond nediamond marked this pull request as ready for review December 2, 2022 01:06
@nediamond
Copy link
Contributor Author

@cretz i reverted the poetry.lock file bc it looked like the checks didnt like it - could use your help generating that since it seems like it may be from a difference in my setup or something

added a super basic readme, but i think this is about as much as i can commit to for now

@cretz
Copy link
Member

cretz commented Dec 2, 2022

could use your help generating that since it seems like it may be from a difference in my setup or something

added a super basic readme, but i think this is about as much as i can commit to for now

Ok, I will look to fixing that and improving your README

@cretz cretz merged commit 8e753f4 into temporalio:main Dec 2, 2022
@nathanielobrown
Copy link

Hey @nediamond and @cretz, I just wanted to share a pitfall I found in this approach for implementing Sentry via interceptors when using threaded activities. Because activities are run in a thread in a ThreadPoolExecutor or in a separate process but the interceptor is run in the main thread, the activity code gets a totally different Sentry scope/context than the interceptor. So any context generated inside the activity (such as breadcrumbs, setting tags, context etc.) will not be included in the resulting Sentry issue. I also was experiencing some "leakage" of contexts, where transaction names would be wrong (I don't 100% understand what was happening there). I haven't tested with async activities, but I feel like maybe they might have the same issue too because Sentry uses contextvars?

My solution was to add Sentry handling to a custom decorator that wraps the @activity.defn decorator and does Sentry handling in the thread. I left the activity interceptor as a fallback, and set an attribute on the activity function and skip interceptor Sentry handling if this attribute is set.

I just wanted to write this up in case you had any further thoughts on this and as a note for others implementing Sentry with Temporal Python SDK.

@cretz
Copy link
Member

cretz commented Jan 30, 2023

Hrmm, this is true. I think we need to be putting contextvars from interceptors on to threaded activities. I have opened temporalio/sdk-python#263.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants