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

Add instrument_celery method #322

Merged
merged 21 commits into from
Jul 24, 2024
Merged

Add instrument_celery method #322

merged 21 commits into from
Jul 24, 2024

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Jul 18, 2024

No description provided.

Copy link

cloudflare-workers-and-pages bot commented Jul 18, 2024

Deploying logfire-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 82d52b5
Status: ✅  Deploy successful!
Preview URL: https://a64bba16.logfire-docs.pages.dev
Branch Preview URL: https://add-celery-instrumentation.logfire-docs.pages.dev

View logs

@Kludex
Copy link
Member Author

Kludex commented Jul 18, 2024

I don't understand how I broke the tests... 🤔

@Kludex Kludex force-pushed the add-celery-instrumentation branch from 3f01a65 to 69e5321 Compare July 18, 2024 13:56
Copy link

codecov bot commented Jul 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@Kludex Kludex requested a review from alexmojaki July 18, 2024 14:39
docs/integrations/celery.md Show resolved Hide resolved
tests/otel_integrations/test_celery.py Outdated Show resolved Hide resolved
@@ -87,9 +87,52 @@ jobs:
name: coverage-${{ matrix.python-version }}
path: coverage

test-integration:
Copy link
Contributor

Choose a reason for hiding this comment

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

i really don't like that so much has been copied here. can we just have one workflow, with separate steps for unit and integration tests?

Copy link
Member Author

@Kludex Kludex Jul 19, 2024

Choose a reason for hiding this comment

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

I think having a different job is a way to "motivate" us to write more tests like those, since it's going to eventually slow us down.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your complaint seems more about the amount of duplicated code, rather than the idea of having a separate flow. Which can be solved by creating a common GH action.

@pytest.fixture(autouse=True)
def celery_worker(celery_app: Celery) -> Iterator[WorkController]:
logger = logging.getLogger()
with start_worker(celery_app, perform_ping_check=False, loglevel=logger.level) as worker: # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

The loglevel has a default value to ERROR, and it modifies the root logger.

This code just sets the current level.

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be in the code

@Kludex Kludex requested a review from alexmojaki July 21, 2024 17:37
@Kludex Kludex merged commit 1722546 into main Jul 24, 2024
13 checks passed
@Kludex Kludex deleted the add-celery-instrumentation branch July 24, 2024 12:44
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.

2 participants