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

[Python] Updated celery instrumentation documentation #11600

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

antonpirker
Copy link
Member

@antonpirker antonpirker commented Oct 17, 2024

DESCRIBE YOUR PR

Better description on how and where to setup the Python SDK when you use Celery.

Preview Link:
https://sentry-docs-git-antonpirker-pythoncelery-instrumentation.sentry.dev/platforms/python/integrations/celery/

Fixes #9955

IS YOUR CHANGE URGENT?

Help us prioritize incoming PRs by letting us know when the change needs to go live.

  • Urgent deadline (GA date, etc.):
  • Other deadline:
  • None: Not urgent, can wait up to 1 week+

SLA

  • Teamwork makes the dream work, so please add a reviewer to your PRs.
  • Please give the docs team up to 1 week to review your PR unless you've added an urgent due date to it.
    Thanks in advance for your help!

PRE-MERGE CHECKLIST

Make sure you've checked the following before merging your changes:

  • Checked Vercel preview for correctness, including links
  • PR was reviewed and approved by any necessary SMEs (subject matter experts)
  • PR was reviewed and approved by a member of the Sentry docs team

LEGAL BOILERPLATE

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

EXTRA RESOURCES

Copy link

codecov bot commented Oct 17, 2024

Bundle Report

Changes will decrease total bundle size by 15 bytes (-0.0%) ⬇️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
sentry-docs-server-cjs 7.52MB 6 bytes (-0.0%) ⬇️
sentry-docs-edge-server-array-push 257.08kB 3 bytes (-0.0%) ⬇️
sentry-docs-client-array-push 6.44MB 6 bytes (-0.0%) ⬇️

Copy link

vercel bot commented Oct 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
changelog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 17, 2024 1:46pm
sentry-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 17, 2024 1:46pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
develop-docs ⬜️ Ignored (Inspect) Oct 17, 2024 1:46pm

Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

I think this is generally an improvement, but I am confused by parts of your proposed changes. Please see comments


### Standalone Setup
To get the most out of Sentry make sure to initialize the Sentry SDK in your Celery worker processes as well as your application that is sending messages to Celery.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can make this clearer and more concise:

Suggested change
To get the most out of Sentry make sure to initialize the Sentry SDK in your Celery worker processes as well as your application that is sending messages to Celery.
When using Celery without Django, you need to initialize the Sentry SDK in both your application and the Celery worker process.


If you're using Celery standalone, there are two ways to set this up:
In addition to capturing errors, you can monitor interactions between multiple services or applications by [enabling tracing](/concepts/key-terms/tracing/). You can also collect and analyze performance profiles from real users with [profiling](/product/explore/profiling/).
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure we need this paragraph – I think this would belong more on a page with general information about the Sentry SDK.

Unless we want to highlight something specific to Celery, I would delete this.

Suggested change
In addition to capturing errors, you can monitor interactions between multiple services or applications by [enabling tracing](/concepts/key-terms/tracing/). You can also collect and analyze performance profiles from real users with [profiling](/product/explore/profiling/).


- Initializing the SDK in the configuration file loaded with Celery's `--config` parameter
- Initializing the SDK by hooking it to either the [`celeryd_init`](https://docs.celeryq.dev/en/stable/userguide/signals.html?#celeryd-init) or [`worker_init`](https://docs.celeryq.dev/en/stable/userguide/signals.html?#worker-init) signals
Select which Sentry features you'd like to install in addition to Error Monitoring to get the corresponding installation and configuration instructions below.
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, I think this should be obvious.

Suggested change
Select which Sentry features you'd like to install in addition to Error Monitoring to get the corresponding installation and configuration instructions below.

app = Celery("tasks", broker="...")

# Initialize Sentry SDK on Celery startup
@signals.celeryd_init.connect
Copy link
Member

Choose a reason for hiding this comment

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

Aren't both signals needed when starting the Celery working in a separate process?

return x + y
```

The [`celeryd_init`](https://docs.celeryq.dev/en/stable/userguide/signals.html?#celeryd-init) signal is triggered when the Celery deamon is started, before the worker processes are spawned. You can use the [`worker_init`](https://docs.celeryq.dev/en/stable/userguide/signals.html?#worker-init) signal instead if you want to initialize Sentry on start of each worker process.
Copy link
Member

Choose a reason for hiding this comment

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

See comment above. If I remember correctly (it has been a while since I played around with Celery), both signals are needed when using a separate Celery worker process (which is recommended). So, this is not an either/or thing, but rather both are needed when using the separate worker process.


The [`celeryd_init`](https://docs.celeryq.dev/en/stable/userguide/signals.html?#celeryd-init) signal is triggered when the Celery deamon is started, before the worker processes are spawned. You can use the [`worker_init`](https://docs.celeryq.dev/en/stable/userguide/signals.html?#worker-init) signal instead if you want to initialize Sentry on start of each worker process.

#### Setup in Your Application
Copy link
Member

Choose a reason for hiding this comment

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

I am confused – why is this section separate from the previous one? Is this why you are suggesting using only one of the signals?

This kinda makes it seem like the application and Celery stuff are completely separate. I find it confusing, but maybe it makes sense to users? Really not sure


## Verify

To verify if your SDK is initialized on worker start, you can pass `debug=True` to `sentry_sdk.init()` to see extra output when the SDK is initialized. If the output appears during worker startup and not only after a task has started, then it's working properly.
To verify if your SDK is initialized on worker start, you can pass `debug=True` to `sentry_sdk.init()` to see extra output in your Celery logs when the SDK is initialized. If the output appears during worker startup and not only after a task has started, then it's working properly.
Copy link
Member

Choose a reason for hiding this comment

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

Should we perhaps add an example of how to get an error in Sentry to this section, like how we do for many of our other integrations? (potentially out of scope for this PR)

Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

One more nit


<PlatformContent includePath="getting-started-config" />
### Setup Celery (Without Django)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### Setup Celery (Without Django)
### Setup Celery Without Django

Remove parentheses to be consistent with the "Setup Celery With Django" section. Alternatively, we could rename the "Setup Celery With Django" section to "Setup Celery (With Django)."

@antonpirker antonpirker requested a review from a team October 21, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify Celery integration init docs
2 participants