8000 Fix tracing tests by sl0thentr0py · Pull Request #3799 · getsentry/sentry-python · GitHub
[go: up one dir, main page]
Skip to content

Fix tracing tests #3799

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
Merged
merged 1 commit into from
Nov 18, 2024
Merged

Fix tracing tests #3799

merged 1 commit into from
Nov 18, 2024

Conversation

sl0thentr0py
Copy link
Member

leaving comments in code for explanation

@sl0thentr0py sl0thentr0py force-pushed the neel/potel/fix-tracing-tests branch from d31a36c to c5e4a6d Compare November 18, 2024 15:53
@codecov Codecov
Copy link
codecov bot commented Nov 18, 2024 •
edited
Loading

❌ 1075 Tests Failed:

Tests completed Failed Passed Skipped
19731 1075 18656 4411
View the top 1 failed tests by shortest run time 
tests.test_scope test_with_isolation_scope
Stack Traces | 0.001s run time 
tests/test_scope.py:357: in test_with_isolation_scope
    assert scope is in_with_isolation_scope
E   assert <PotelScope id=0x7f0fc213e7b0 name=None type=ScopeType.ISOLATION> is <Scope id=0x7f0fc579ece0 name=None type=ScopeType.ISOLATION>
View the full list of 2 ❄️ flaky tests 
tests.test_scope test_with_isolation_scope_data

Flake rate in main: 99.20% (Passed 2 times, Failed 248 times)

Stack Traces | 0.001s run time 
tests/test_scope.py:387: in test_with_isolation_scope_data
    assert scope._tags == {"before_isolation_scope": 1}
E   AssertionError: assert {} == {'before_isolation_scope': 1}
E     
E     Right contains 1 more item:
E     {'before_isolation_scope': 1}
E     
E     Full diff:
E     + {}
E     - {
E     -     'before_isolation_scope': 1,
E     - }
tests.test_scope test_with_new_scope

Flake rate in main: 96.79% (Passed 9 times, Failed 271 times)

Stack Traces | 0.001s run time 
tests/test_scope.py:587: in test_with_new_scope
    assert scope is in_with_current_scope
E   assert <PotelScope id=0x7fb2b8fadb80 name=None type=ScopeType.CURRENT> is <Scope id=0x7fb2b8ed1740 name=None type=ScopeType.CURRENT>

To view more test analytics, go to the Test Analytics Dashboard
Got feedback? Let us know on Github

sl0thentr0py
sl0thentr0py commented Nov 18, 2024
View reviewed changes
sentry_sdk/scope.py
@@ -786,14 +786,6 @@ def span(self, span):
# type: (Optional[Span]) -> None
"""Set current tracing span."""
self._span = span
# XXX: this differs from the implementation in JS, there Scope.setSpan
Copy link
Member Author

Choose a reason for hiding this comment

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

removing this opaque behavior, we will not rely on it anymore

sentry_sdk/integrations/opentelemetry/sampler.py
if parent_context.trace_flags.sampled:
return True
if parent_context.trace_flags.sampled is not None:
return parent_context.trace_flags.sampled
Copy link
Member Author

Choose a reason for hiding this comment

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

fixes bug in the unsampled case

sentry_sdk/tracing.py
@@ -1398,7 +1399,9 @@ def span_id(self):
@property
def is_valid(self):
# type: () -> bool
return self._otel_span.get_span_context().is_valid
return self._otel_span.get_span_context().is_valid and isinstance(
Copy link
Member Author

Choose a reason for hiding this comment

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

fixes another twp bug

tests/tracing/test_integration_tests.py
@pytest.mark.parametrize("sample_rate", [0.0, 1.0])
def test_continue_from_headers(sentry_init, capture_envelopes, sampled, sample_rate):
def test_continue_trace(
Copy link
Member Author

Choose a reason for hiding this comment

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

simplified this test since it was too confusing and we can write a separate test for explicit sampling later if really wanted

tests/tracing/test_integration_tests.py
"transaction": "Head SDK tx",
"trace_id": trace_id,
}


@pytest.mark.parametrize(
"args,expected_refcount",
Copy link
Member Author

Choose a reason for hiding this comment

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

no longer relevant

tests/tracing/test_misc.py
from sentry_sdk.tracing_utils import should_propagate_trace
from sentry_sdk.utils import Dsn


def test_span_trimming(sentry_init, capture_events):
sentry_init(traces_sample_rate=1.0, _experiments={"max_spans": 3})
Copy link
Member Author

Choose a reason for hiding this comment

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

no longer supported, if we want to support max_spans we will need some logic in the span processor but it is not simple to support it

tests/tracing/test_misc.py
@@ -82,197 +58,31 @@ def test_transaction_data(sentry_init, capture_events):
assert span_data.items() >= {"spanfoo": "spanbar"}.items()


def test_start_transaction(sentry_init):
Copy link
Member Author

Choose a reason for hiding this comment

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

most of the following tests are too unit-testy and don't add much or are obsolete.

tests/tracing/test_propagation.py
import pytest


def test_standalone_span_iter_headers(sentry_init):
Copy link
Member Author

Choose a reason for hiding this comment

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

there is no longer a difference between a standalone span and a transaction so this is obsolete

tests/tracing/test_sampling.py
pass

assert len(events) == 1


@pytest.mark.parametrize("sampling_decision", [True, False])
Copy link
Member Author

Choose a reason for hiding this comment

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

no longer true, since unsampled spans can't have attributes and plus I don't see what the point of this test is

tests/tracing/test_span_name.py
import sentry_sdk


def test_start_span_description(sentry_init, capture_events):
Copy link
Member Author

Choose a reason for hiding this comment

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

we will remove span description completely since it was already deprecated

sentrivana
sentrivana approved these changes Nov 18, 2024
8000
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.

see couple suggestions, else lgtm

@sl0thentr0py sl0thentr0py force-pushed the neel/potel/fix-tracing-tests branch from c5e4a6d to 5e6dbbb Compare November 18, 2024 17:00
@sl0thentr0py sl0thentr0py merged commit 6afa91c into potel-base Nov 18, 2024
27 of 124 checks passed
@sl0thentr0py sl0thentr0py deleted the neel/potel/fix-tracing-tests branch November 18, 2024 17:13
@sl0thentr0py sl0thentr0py mentioned this pull request Nov 18, 2024
Draft
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
None yet
Projects
None yet
Milestone
No milestone
Development

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