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

Fix ray tests in POTel #3877

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

Fix ray tests in POTel #3877

wants to merge 1 commit into from
+39 −28

Conversation

antonpirker
Copy link
Member

Make sure to set the transaction name.

@antonpirker antonpirker requested review from sl0thentr0py and sentrivana and removed request for sl0thentr0py December 17, 2024 12:05
@codecov Codecov
Copy link

codecov bot commented Dec 17, 2024 •
edited
Loading

❌ 257 Tests Failed:

Tests completed Failed Passed Skipped
12738 257 12481 3318
View the top 3 failed tests by shortest run time 
 tests.integrations.quart.test_quart
Stack Traces | 0s run time 
No failure message available
 tests.integrations.quart.test_quart
Stack Traces | 0s run time 
No failure message available
tests.integrations.arq.test_arq test_job_retry[init_arq]
Stack Traces | 0.064s run time 
.../integrations/arq/test_arq.py:180: in test_job_retry
    assert event["contexts"]["trace"]["status"] == "aborted"
E   AssertionError: assert 'ok' == 'aborted'
E     - aborted
E     + ok

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

sentrivana
sentrivana approved these changes Dec 17, 2024
View reviewed changes
Copy link
Contributor

@sentrivana sentrivana left a comment

Choose a reason for hiding this comment

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

lgtm, see one small comment

sentry_sdk/integrations/ray.py
Comment on lines +64 to +67
sentry_sdk.get_current_scope().set_transaction_name(
root_span_name,
source=TRANSACTION_SOURCE_TASK,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work before we even started the transaction? Maybe logically it'd better fit inside the root_span context manager?

Copy link
Member Author

@antonpirker antonpirker Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Because this sets the transaction name only on the scope, this should be fine.

The other question this brought up inside me is why the ray integration does no isolation scope forking anywhere...

I will test this tomorrow.

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

@sentrivana sentrivana sentrivana approved these changes

Assignees
No one assigned
Labels
Project: POTel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@antonpirker @sentrivana