-
Notifications
You must be signed in to change notification settings - Fork 608
feat(httpx): Migrate to span first #6084
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
base: master
Are you sure you want to change the base?
Changes from all commits
b47362d
019fa47
6815bc6
bf39690
25a953e
9d2546f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -278,12 +278,9 @@ def __init__( | |
| self._start_timestamp = datetime.now(timezone.utc) | ||
| self._timestamp: "Optional[datetime]" = None | ||
|
|
||
| try: | ||
| # profiling depends on this value and requires that | ||
| # it is measured in nanoseconds | ||
| self._start_timestamp_monotonic_ns = nanosecond_time() | ||
| except AttributeError: | ||
| pass | ||
| # profiling depends on this value and requires that | ||
| # it is measured in nanoseconds | ||
| self._start_timestamp_monotonic_ns = nanosecond_time() | ||
|
Comment on lines
+281
to
+283
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For context, this code including the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
From my commit splunking, it looks like this try-except was introduced a while ago in the existing tracing implementation when Python 2 was still supported, and the Since we've dropped support for Python 2 within the SDK, this is safe to remove 🔥 |
||
|
|
||
| self._span_id: "Optional[str]" = None | ||
|
|
||
|
|
@@ -385,12 +382,12 @@ def _end(self, end_timestamp: "Optional[Union[float, datetime]]" = None) -> None | |
| ) | ||
|
|
||
| if self._timestamp is None: | ||
| try: | ||
| if self._start_timestamp_monotonic_ns is not None: | ||
| elapsed = nanosecond_time() - self._start_timestamp_monotonic_ns | ||
| self._timestamp = self._start_timestamp + timedelta( | ||
| microseconds=elapsed / 1000 | ||
| ) | ||
| except AttributeError: | ||
| else: | ||
| self._timestamp = datetime.now(timezone.utc) | ||
|
|
||
| client = sentry_sdk.get_client() | ||
|
|
@@ -467,6 +464,10 @@ def sampled(self) -> "Optional[bool]": | |
| def start_timestamp(self) -> "Optional[datetime]": | ||
| return self._start_timestamp | ||
|
|
||
| @property | ||
| def start_timestamp_monotonic_ns(self) -> "Optional[int]": | ||
| return self._start_timestamp_monotonic_ns | ||
|
ericapisani marked this conversation as resolved.
Comment on lines
+467
to
+469
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's not expose new properties on the span -- the idea is to keep the API surface minimal. We can still use the attribute directly in our code if we need to, but I think we don't (see other comments). |
||
|
|
||
| @property | ||
| def timestamp(self) -> "Optional[datetime]": | ||
| return self._timestamp | ||
|
|
@@ -681,6 +682,10 @@ def sampled(self) -> "Optional[bool]": | |
| def start_timestamp(self) -> "Optional[datetime]": | ||
| return None | ||
|
|
||
| @property | ||
| def start_timestamp_monotonic_ns(self) -> "Optional[int]": | ||
| return None | ||
|
|
||
| @property | ||
| def timestamp(self) -> "Optional[datetime]": | ||
| return None | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Please see my comments on the async send since this is essentially the same changeset