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.

Sign up for GitHub

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

Jump to bottom

add celery integration #124

Merged
merged 4 commits into from
Dec 10, 2022
Merged

add celery integration #124

merged 4 commits into from
Dec 10, 2022

Conversation

Mng-dev-ai
Copy link
Contributor

@Mng-dev-ai Mng-dev-ai commented Nov 21, 2022 •
edited by Kelvin4664
Loading

Resolves #61

@subzero10
Copy link
Member

@Kelvin4664 Can you please take a look at this? Thanks!

Mng-dev-ai
Mng-dev-ai commented Nov 27, 2022
View reviewed changes
setup.py Outdated Show resolved Hide resolved
Kelvin4664
Kelvin4664 approved these changes Dec 4, 2022
View reviewed changes
Copy link
Collaborator

@Kelvin4664 Kelvin4664 left a comment

Choose a reason for hiding this comment

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

This looks good. Particularly love the way you hooked into celery events and not rely on monkey patching.

Please add some integration instruction to Readme and a Changelog entry under the Unreleased header.

Thank you!

honeybadger/contrib/celery.py Outdated Show resolved Hide resolved
honeybadger/contrib/celery.py Show resolved Hide resolved
Kelvin4664
Kelvin4664 reviewed Dec 4, 2022
View reviewed changes
README.md Outdated Show resolved Hide resolved
Kelvin4664
Kelvin4664 reviewed Dec 4, 2022
View reviewed changes
README.md
HONEYBADGER_API_KEY= 'production',
HONEYBADGER_ENVIRONMENT= '<your key>'
)
CeleryHoneybadger(app, report_exceptions=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have mixed feeling around having report_exceptions flag separate from the the other config (api_key and environment)
is there a reason for the separation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed the same pattern you guys used in the Flask plugin

@Mng-dev-ai
Copy link
Contributor Author

@Kelvin4664 are the tests failing because of my pr? I can see It's coming from fastapi

subzero10
subzero10 reviewed Dec 5, 2022
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
@subzero10
Copy link
Member

@Kelvin4664 are the tests failing because of my pr? I can see It's coming from fastapi

Hey @Mng-dev-ai, have you tried running them locally?
What if you checkout master branch and run them? I can see here that they passed last time they ran on master.

@Mng-dev-ai
Copy link
Contributor Author

Hey @subzero10, ya I tried running them on the master branch locally and got the same error

@subzero10
Copy link
Member

subzero10 commented Dec 9, 2022 •
edited
Loading

Hey @subzero10, ya I tried running them on the master branch locally and got the same error

Found the issue. It seems that we need to install httpx with the latest version of fastapi.
#126

@subzero10
Copy link
Member

Hey @Mng-dev-ai, can you please rebase/merge with master? This should fix the current issue with the tests.

@Mng-dev-ai
Copy link
Contributor Author

@subzero10 Done.

@subzero10 subzero10 merged commit 408432f into honeybadger-io:master Dec 10, 2022
@subzero10
Copy link
Member

Published with v0.14.0!

Thank you @Mng-dev-ai and @Kelvin4664!

@28140
Copy link

28140 commented Dec 12, 2022

Release v0.14.0 is broken!
It has an unmet dependency: celery.
Go figure!

@subzero10
Copy link
Member

subzero10 commented Dec 13, 2022 •
edited
Loading

Release v0.14.0 is broken! It has an unmet dependency: celery. Go figure!

😱 @Mng-dev-ai Can you make the plugin conditional? Something like:

try:
    from honeybadger.contrib.celery import CeleryHoneybadger
    hasCelery = True
except ImportError as err:
    hasCelery = False

Or is there a better approach for this?

cc @Kelvin4664

@subzero10 subzero10 mentioned this pull request Dec 13, 2022
Closed
@Mng-dev-ai
Copy link
Contributor Author

Mng-dev-ai commented Dec 13, 2022 •
edited
Loading

@subzero10 oh right, added a PR to fix it here

@subzero10
Copy link
Member

@subzero10 oh right, added a PR to fix it here

That was fast, thanks! I published v0.14.1 with your fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@subzero10 subzero10 subzero10 left review comments

@Kelvin4664 Kelvin4664 Kelvin4664 approved these changes

@BethanyBerkowitz BethanyBerkowitz Awaiting requested review from BethanyBerkowitz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Celery Integration
4 participants
@Mng-dev-ai @subzero10 @28140 @Kelvin4664