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

Record exception in Celery instrumentation #1573

Merged
merged 14 commits into from
Feb 3, 2023

Conversation

tombruijn
Copy link
Contributor

@tombruijn tombruijn commented Jan 11, 2023

Description

Add tests for errors in Celery tasks

I noticed there were no tests for the error scenario in the Celery
package. This commit adds a basic test, based on the previous test and
how I see other packages test the error status on the span.

Part of #987

Record exception in Celery instrumentation

In addition to setting the status on the span, also record the exception
on the span. This adds an event to the span with more details about the
error, following the format other instrumentations also use.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • tox -e test-instrumentation-celery

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added

I noticed there were no tests for the error scenario in the Celery
package. This commit adds a basic test, based on the previous test and
how I see other packages test the error status on the span.

Part of open-telemetry#987
In addition to setting the status on the span, also record the exception
on the span. This adds an event to the span with more details about the
error, following the format other instrumentations also use.
@tombruijn tombruijn requested a review from a team January 11, 2023 07:35
@shalevr
Copy link
Member

shalevr commented Jan 11, 2023

Looking great!

@tombruijn
Copy link
Contributor Author

I'm pretty sure I fixed the lint failures. I can't reliably run the linter locally though to confirm.

@shalevr
Copy link
Member

shalevr commented Jan 16, 2023

You can run just the python code formatter, it will fix most of the problems
"black --config pyproject.toml . "

@tombruijn
Copy link
Contributor Author

Thanks! That's a lot quicker than running tox -e lint.

Does this PR also need an update to the docker-tests? Like you mentioned in the Redis query sanitization PR? #1572 (comment)

@shalevr
Copy link
Member

shalevr commented Jan 16, 2023

Thanks! That's a lot quicker than running tox -e lint.

Does this PR also need an update to the docker-tests? Like you mentioned in the Redis query sanitization PR? #1572 (comment)

There is no need to add docket-tests in this case, In your last PR you changed the default behavior so you had to update all Redis tests(including the docker tests)

@tombruijn
Copy link
Contributor Author

Great! Then this PR is ready for review.

CHANGELOG.md Outdated Show resolved Hide resolved
The celery tests failed on Python 3.11. This is most likely due to this
issue in billiard, a celery dependency, about it not working on Python
3.11 because of the error reported in the CI:
celery/billiard#377

It's been fixed in billiard 4.1.0, but celery is locked on billiard
version lower than 4, so it cannot use this version with the fix.

This issue does not arise on the Docker tests, because they use Python
3.9.16.

I've moved the error test span event assertions to the error test that
is available in the functional tests, and removed the unit test. That
way, the build will run successfully.
This was added in a recent merge commit on this PR branch.
@tombruijn
Copy link
Contributor Author

@srikanthccv I've updated the PR to fix the failing CI. It's become of some dependency issue described in 2888c12.

I've moved the error test assertion to the functional tests so the build is green.

Can you run the build again?

srikanthccv and others added 2 commits January 24, 2023 07:50
With the move of the tests for tasks with errors to the functional
tests, remove the unit test's error task and unused imports.
@tombruijn
Copy link
Contributor Author

@srikanthccv I'm learning a lot about what checks need to pass. (I can't run the tox lint task in its entirety locally (some C extension error), so it's process.) The flake8 output contains many issues that do not apply to this PR. I fixed what was related to the celery instrumentation. I hope that's enough. Please try the build again. I really hope it will now be green.

@srikanthccv
Copy link
Member

If you think we can improve/document the steps based on your experience, please suggest them. We want our contributing experience to be better. Whenever I try to reproduce some of these things, I fail because sometimes the errors occur in specific env.

@tombruijn
Copy link
Contributor Author

tombruijn commented Jan 30, 2023

@srikanthccv I asked for help on Slack and Diego was able to help out, resulting in PR #1604. I may send in a PR or two for other things I ran into.

Edit: blocked on #1609

@srikanthccv srikanthccv enabled auto-merge (squash) February 3, 2023 03:20
@srikanthccv srikanthccv merged commit 673e4aa into open-telemetry:main Feb 3, 2023
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.

3 participants