-
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 3 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 | ||||
|---|---|---|---|---|---|---|
| @@ -1,12 +1,15 @@ | ||||||
| import sentry_sdk | ||||||
| from sentry_sdk import start_span | ||||||
| from sentry_sdk import start_span as legacy_start_span | ||||||
| from sentry_sdk.consts import OP, SPANDATA | ||||||
| from sentry_sdk.integrations import Integration, DidNotEnable | ||||||
| from sentry_sdk.traces import start_span | ||||||
| from sentry_sdk.tracing import BAGGAGE_HEADER_NAME | ||||||
| from sentry_sdk.tracing_utils import ( | ||||||
| add_http_request_source_for_streamed_span, | ||||||
| should_propagate_trace, | ||||||
| add_http_request_source, | ||||||
| add_sentry_baggage_to_headers, | ||||||
| has_span_streaming_enabled, | ||||||
| ) | ||||||
| from sentry_sdk.utils import ( | ||||||
| SENSITIVE_DATA_SUBSTITUTE, | ||||||
|
|
@@ -20,6 +23,7 @@ | |||||
|
|
||||||
| if TYPE_CHECKING: | ||||||
| from typing import Any | ||||||
| from sentry_sdk._types import Attributes | ||||||
|
|
||||||
|
|
||||||
| try: | ||||||
|
|
@@ -49,48 +53,99 @@ | |||||
|
|
||||||
| @ensure_integration_enabled(HttpxIntegration, real_send) | ||||||
| def send(self: "Client", request: "Request", **kwargs: "Any") -> "Response": | ||||||
| client = sentry_sdk.get_client() | ||||||
| is_span_streaming_enabled = has_span_streaming_enabled(client.options) | ||||||
|
|
||||||
| parsed_url = None | ||||||
| with capture_internal_exceptions(): | ||||||
| parsed_url = parse_url(str(request.url), sanitize=False) | ||||||
|
|
||||||
| with start_span( | ||||||
| op=OP.HTTP_CLIENT, | ||||||
| name="%s %s" | ||||||
| % ( | ||||||
| request.method, | ||||||
| parsed_url.url if parsed_url else SENSITIVE_DATA_SUBSTITUTE, | ||||||
| ), | ||||||
| origin=HttpxIntegration.origin, | ||||||
| ) as span: | ||||||
| span.set_data(SPANDATA.HTTP_METHOD, request.method) | ||||||
| if parsed_url is not None: | ||||||
| span.set_data("url", parsed_url.url) | ||||||
| span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query) | ||||||
| span.set_data(SPANDATA.HTTP_FRAGMENT, parsed_url.fragment) | ||||||
|
|
||||||
| if should_propagate_trace(sentry_sdk.get_client(), str(request.url)): | ||||||
| for ( | ||||||
| key, | ||||||
| value, | ||||||
| ) in sentry_sdk.get_current_scope().iter_trace_propagation_headers(): | ||||||
| logger.debug( | ||||||
| "[Tracing] Adding `{key}` header {value} to outgoing request to {url}.".format( | ||||||
| key=key, value=value, url=request.url | ||||||
| if is_span_streaming_enabled: | ||||||
| with start_span( | ||||||
| name="%s %s" | ||||||
| % ( | ||||||
| request.method, | ||||||
| parsed_url.url if parsed_url else SENSITIVE_DATA_SUBSTITUTE, | ||||||
| ), | ||||||
| attributes={ | ||||||
| "sentry.op": OP.HTTP_CLIENT, | ||||||
| "sentry.origin": HttpxIntegration.origin, | ||||||
| SPANDATA.HTTP_METHOD: request.method, | ||||||
| }, | ||||||
| ) as segment: | ||||||
| attributes: "Attributes" = {} | ||||||
|
|
||||||
| if parsed_url is not None: | ||||||
| attributes["url"] = parsed_url.url | ||||||
| attributes[SPANDATA.HTTP_QUERY] = parsed_url.query | ||||||
| attributes[SPANDATA.HTTP_FRAGMENT] = parsed_url.fragment | ||||||
|
|
||||||
| if should_propagate_trace(client, str(request.url)): | ||||||
| for ( | ||||||
| key, | ||||||
| value, | ||||||
| ) in ( | ||||||
| sentry_sdk.get_current_scope().iter_trace_propagation_headers() | ||||||
| ): | ||||||
| logger.debug( | ||||||
| f"[Tracing] Adding `{key}` header {value} to outgoing request to {request.url}." | ||||||
| ) | ||||||
| ) | ||||||
|
|
||||||
| if key == BAGGAGE_HEADER_NAME: | ||||||
| add_sentry_baggage_to_headers(request.headers, value) | ||||||
| else: | ||||||
| request.headers[key] = value | ||||||
| if key == BAGGAGE_HEADER_NAME: | ||||||
| add_sentry_baggage_to_headers(request.headers, value) | ||||||
| else: | ||||||
| request.headers[key] = value | ||||||
|
|
||||||
| rv = real_send(self, request, **kwargs) | ||||||
| rv = real_send(self, request, **kwargs) | ||||||
|
|
||||||
| span.set_http_status(rv.status_code) | ||||||
| span.set_data("reason", rv.reason_phrase) | ||||||
| segment.status = "error" if rv.status_code >= 400 else "ok" | ||||||
| attributes[SPANDATA.HTTP_STATUS_CODE] = rv.status_code | ||||||
|
|
||||||
| with capture_internal_exceptions(): | ||||||
| add_http_request_source(span) | ||||||
| segment.set_attributes(attributes) | ||||||
|
Check warning on line 104 in sentry_sdk/integrations/httpx.py
|
||||||
|
ericapisani marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| # Needs to happen within the context manager as we want to attach the | ||||||
| # final data before the span finishes and is sent for ingesting. | ||||||
| with capture_internal_exceptions(): | ||||||
| add_http_request_source_for_streamed_span(segment) | ||||||
| else: | ||||||
| with legacy_start_span( | ||||||
|
ericapisani marked this conversation as resolved.
Outdated
|
||||||
| op=OP.HTTP_CLIENT, | ||||||
| name="%s %s" | ||||||
| % ( | ||||||
| request.method, | ||||||
| parsed_url.url if parsed_url else SENSITIVE_DATA_SUBSTITUTE, | ||||||
| ), | ||||||
| origin=HttpxIntegration.origin, | ||||||
| ) as span: | ||||||
| span.set_data(SPANDATA.HTTP_METHOD, request.method) | ||||||
| if parsed_url is not None: | ||||||
| span.set_data("url", parsed_url.url) | ||||||
| span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query) | ||||||
| span.set_data(SPANDATA.HTTP_FRAGMENT, parsed_url.fragment) | ||||||
|
|
||||||
| if should_propagate_trace(client, str(request.url)): | ||||||
| for ( | ||||||
| key, | ||||||
| value, | ||||||
| ) in ( | ||||||
| sentry_sdk.get_current_scope().iter_trace_propagation_headers() | ||||||
| ): | ||||||
| logger.debug( | ||||||
| f"[Tracing] Adding `{key}` header {value} to outgoing request to {request.url}." | ||||||
| ) | ||||||
|
|
||||||
| if key == BAGGAGE_HEADER_NAME: | ||||||
| add_sentry_baggage_to_headers(request.headers, value) | ||||||
| else: | ||||||
| request.headers[key] = value | ||||||
|
|
||||||
| rv = real_send(self, request, **kwargs) | ||||||
|
|
||||||
| span.set_http_status(rv.status_code) | ||||||
| span.set_data("reason", rv.reason_phrase) | ||||||
|
|
||||||
| with capture_internal_exceptions(): | ||||||
| add_http_request_source(span) | ||||||
|
|
||||||
| return rv | ||||||
|
|
||||||
|
|
@@ -103,50 +158,100 @@ | |||||
| async def send( | ||||||
| self: "AsyncClient", request: "Request", **kwargs: "Any" | ||||||
| ) -> "Response": | ||||||
| if sentry_sdk.get_client().get_integration(HttpxIntegration) is None: | ||||||
| client = sentry_sdk.get_client() | ||||||
| if client.get_integration(HttpxIntegration) is None: | ||||||
| return await real_send(self, request, **kwargs) | ||||||
|
|
||||||
| is_span_streaming_enabled = has_span_streaming_enabled(client.options) | ||||||
| parsed_url = None | ||||||
| with capture_internal_exceptions(): | ||||||
| parsed_url = parse_url(str(request.url), sanitize=False) | ||||||
|
|
||||||
| with start_span( | ||||||
| op=OP.HTTP_CLIENT, | ||||||
| name="%s %s" | ||||||
| % ( | ||||||
| request.method, | ||||||
| parsed_url.url if parsed_url else SENSITIVE_DATA_SUBSTITUTE, | ||||||
| ), | ||||||
| origin=HttpxIntegration.origin, | ||||||
| ) as span: | ||||||
| span.set_data(SPANDATA.HTTP_METHOD, request.method) | ||||||
| if parsed_url is not None: | ||||||
| span.set_data("url", parsed_url.url) | ||||||
| span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query) | ||||||
| span.set_data(SPANDATA.HTTP_FRAGMENT, parsed_url.fragment) | ||||||
|
|
||||||
| if should_propagate_trace(sentry_sdk.get_client(), str(request.url)): | ||||||
| for ( | ||||||
| key, | ||||||
| value, | ||||||
| ) in sentry_sdk.get_current_scope().iter_trace_propagation_headers(): | ||||||
| logger.debug( | ||||||
| "[Tracing] Adding `{key}` header {value} to outgoing request to {url}.".format( | ||||||
| key=key, value=value, url=request.url | ||||||
| if is_span_streaming_enabled: | ||||||
| with start_span( | ||||||
| name="%s %s" | ||||||
| % ( | ||||||
| request.method, | ||||||
| parsed_url.url if parsed_url else SENSITIVE_DATA_SUBSTITUTE, | ||||||
| ), | ||||||
| attributes={ | ||||||
| "sentry.op": OP.HTTP_CLIENT, | ||||||
| "sentry.origin": HttpxIntegration.origin, | ||||||
| SPANDATA.HTTP_METHOD: request.method, | ||||||
|
ericapisani marked this conversation as resolved.
Outdated
|
||||||
| }, | ||||||
| ) as segment: | ||||||
|
ericapisani marked this conversation as resolved.
Outdated
|
||||||
| attributes: "Attributes" = {} | ||||||
|
|
||||||
| if parsed_url is not None: | ||||||
| attributes["url"] = parsed_url.url | ||||||
|
ericapisani marked this conversation as resolved.
Outdated
|
||||||
| attributes[SPANDATA.HTTP_QUERY] = parsed_url.query | ||||||
|
ericapisani marked this conversation as resolved.
Outdated
|
||||||
| attributes[SPANDATA.HTTP_FRAGMENT] = parsed_url.fragment | ||||||
|
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. Same case as https://github.com/getsentry/sentry-python/pull/6084/changes#r3098336387 -- there's
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. I've asked about |
||||||
|
|
||||||
| if should_propagate_trace(client, str(request.url)): | ||||||
| for ( | ||||||
| key, | ||||||
| value, | ||||||
| ) in ( | ||||||
| sentry_sdk.get_current_scope().iter_trace_propagation_headers() | ||||||
| ): | ||||||
| logger.debug( | ||||||
| f"[Tracing] Adding `{key}` header {value} to outgoing request to {request.url}." | ||||||
| ) | ||||||
| ) | ||||||
| if key == BAGGAGE_HEADER_NAME: | ||||||
| add_sentry_baggage_to_headers(request.headers, value) | ||||||
| else: | ||||||
| request.headers[key] = value | ||||||
|
|
||||||
| rv = await real_send(self, request, **kwargs) | ||||||
| if key == BAGGAGE_HEADER_NAME: | ||||||
| add_sentry_baggage_to_headers(request.headers, value) | ||||||
| else: | ||||||
| request.headers[key] = value | ||||||
|
|
||||||
| span.set_http_status(rv.status_code) | ||||||
| span.set_data("reason", rv.reason_phrase) | ||||||
| rv = await real_send(self, request, **kwargs) | ||||||
|
|
||||||
| with capture_internal_exceptions(): | ||||||
| add_http_request_source(span) | ||||||
| segment.status = "error" if rv.status_code >= 400 else "ok" | ||||||
| attributes[SPANDATA.HTTP_STATUS_CODE] = rv.status_code | ||||||
|
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. https://getsentry.github.io/sentry-conventions/attributes/http/#http-status_code
Suggested change
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. Addressed in 25a953e |
||||||
|
|
||||||
| segment.set_attributes(attributes) | ||||||
|
|
||||||
| # Needs to happen within the context manager as we want to attach the | ||||||
| # final data before the span finishes and is sent for ingesting. | ||||||
| with capture_internal_exceptions(): | ||||||
| add_http_request_source_for_streamed_span(segment) | ||||||
| else: | ||||||
| with legacy_start_span( | ||||||
| op=OP.HTTP_CLIENT, | ||||||
| name="%s %s" | ||||||
| % ( | ||||||
| request.method, | ||||||
| parsed_url.url if parsed_url else SENSITIVE_DATA_SUBSTITUTE, | ||||||
| ), | ||||||
| origin=HttpxIntegration.origin, | ||||||
| ) as span: | ||||||
| span.set_data(SPANDATA.HTTP_METHOD, request.method) | ||||||
| if parsed_url is not None: | ||||||
| span.set_data("url", parsed_url.url) | ||||||
| span.set_data(SPANDATA.HTTP_QUERY, parsed_url.query) | ||||||
| span.set_data(SPANDATA.HTTP_FRAGMENT, parsed_url.fragment) | ||||||
|
|
||||||
| if should_propagate_trace(client, str(request.url)): | ||||||
| for ( | ||||||
| key, | ||||||
| value, | ||||||
| ) in ( | ||||||
| sentry_sdk.get_current_scope().iter_trace_propagation_headers() | ||||||
| ): | ||||||
| logger.debug( | ||||||
| f"[Tracing] Adding `{key}` header {value} to outgoing request to {request.url}." | ||||||
| ) | ||||||
| if key == BAGGAGE_HEADER_NAME: | ||||||
| add_sentry_baggage_to_headers(request.headers, value) | ||||||
| else: | ||||||
| request.headers[key] = value | ||||||
|
|
||||||
| rv = await real_send(self, request, **kwargs) | ||||||
|
|
||||||
| span.set_http_status(rv.status_code) | ||||||
| span.set_data("reason", rv.reason_phrase) | ||||||
|
|
||||||
| with capture_internal_exceptions(): | ||||||
| add_http_request_source(span) | ||||||
|
|
||||||
| return rv | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -283,7 +283,7 @@ def __init__( | |
| # it is measured in nanoseconds | ||
| self._start_timestamp_monotonic_ns = nanosecond_time() | ||
| except AttributeError: | ||
| pass | ||
| self._start_timestamp_monotonic_ns = None | ||
|
|
||
| self._span_id: "Optional[str]" = None | ||
|
|
||
|
|
@@ -385,12 +385,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 +467,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 +685,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