-
Notifications
You must be signed in to change notification settings - Fork 509
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
Fix Celery tests in Potel #3772
base: potel-base
Are you sure you want to change the base?
Conversation
❌ 8883 Tests Failed:
View the top 3 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
assert execution_event["spans"][0] == ApproxDict({ | ||
"trace_id": str(span.trace_id), | ||
"same_process_as_parent": True, | ||
"op": "queue.process", | ||
"description": "dummy_task", | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just makes the diff of failing tests way easier to read
@@ -1582,6 +1582,9 @@ def is_success(self): | |||
|
|||
def finish(self, end_timestamp=None): | |||
# type: (Optional[Union[float, datetime]]) -> Optional[str] | |||
if hasattr(self._otel_span, "status") and self._otel_span.status.status_code == StatusCode.UNSET: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do this in __exit__
above since we are already setting INTERNAL_ERROR
there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if people do not use the contextmanager, the status will not be set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When thinking of it, thats probably ok. If you use the context manager, we take care of the status. If you start/finish spans manually, you need to take care of the status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already doing this in another branch so you can just remove this and pull later
No description provided.