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

Requests library spec fidelity #746

Merged
merged 11 commits into from
Aug 6, 2019
Merged

Requests library spec fidelity #746

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b24abd9
Added util for mapping http status code to grpc status code
victoraugustolls Aug 5, 2019
73ba2fe
Updated requests integration to follow specs
victoraugustolls Aug 5, 2019
e1d8152
Updated changelogs
victoraugustolls Aug 5, 2019
3c6882c
Merge branch 'master' into feature/requests-specs
victoraugustolls Aug 5, 2019
7e2c3e3
Added common exception status
victoraugustolls Aug 6, 2019
524dd23
Simplified http status code to status mapping
victoraugustolls Aug 6, 2019
d5ed389
Now using exceptions mappings
victoraugustolls Aug 6, 2019
b687467
Removed DNS_RESOLUTION_FAILED mapping
victoraugustolls Aug 6, 2019
d62efcc
Fixed exceptions status tests
victoraugustolls Aug 6, 2019
09a060e
Merge branch 'master' into feature/requests-specs
victoraugustolls Aug 6, 2019
2823238
Merge branch 'master' into feature/requests-specs
victoraugustolls Aug 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
## Unreleased
- Added `set_status` to `span`
([#738](https://github.com/census-instrumentation/opencensus-python/pull/738))
- Added `http code` to `grpc code` status code mapping on `utils`
([#746](https://github.com/census-instrumentation/opencensus-python/pull/746))

## 0.7.0
Released 2019-07-31
Expand Down
4 changes: 4 additions & 0 deletions contrib/opencensus-ext-requests/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
# Changelog

## Unreleased
- Added attributes following specs listed [here](https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/HTTP.md#attributes)
([#746](https://github.com/census-instrumentation/opencensus-python/pull/746))
- Fixed span name
([#746](https://github.com/census-instrumentation/opencensus-python/pull/746))

## 0.1.2
Released 2019-04-24
Expand Down
89 changes: 72 additions & 17 deletions contrib/opencensus-ext-requests/opencensus/ext/requests/trace.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from urlparse import urlparse

from opencensus.trace import attributes_helper
from opencensus.trace import exceptions_status
from opencensus.trace import execution_context
from opencensus.trace import span as span_module
from opencensus.trace import utils
Expand All @@ -33,8 +34,12 @@
SESSION_WRAP_METHODS = 'request'
SESSION_CLASS_NAME = 'Session'

HTTP_URL = attributes_helper.COMMON_ATTRIBUTES['HTTP_URL']
HTTP_HOST = attributes_helper.COMMON_ATTRIBUTES['HTTP_HOST']
HTTP_METHOD = attributes_helper.COMMON_ATTRIBUTES['HTTP_METHOD']
HTTP_PATH = attributes_helper.COMMON_ATTRIBUTES['HTTP_PATH']
HTTP_ROUTE = attributes_helper.COMMON_ATTRIBUTES['HTTP_ROUTE']
HTTP_STATUS_CODE = attributes_helper.COMMON_ATTRIBUTES['HTTP_STATUS_CODE']
HTTP_URL = attributes_helper.COMMON_ATTRIBUTES['HTTP_URL']


def trace_integration(tracer=None):
Expand Down Expand Up @@ -74,22 +79,47 @@ def call(url, *args, **kwargs):
if utils.disable_tracing_hostname(dest_url, blacklist_hostnames):
return requests_func(url, *args, **kwargs)

path = parsed_url.path if parsed_url.path else '/'

_tracer = execution_context.get_opencensus_tracer()
_span = _tracer.start_span()
_span.name = '[requests]{}'.format(requests_func.__name__)
_span.name = '{}'.format(path)
_span.span_kind = span_module.SpanKind.CLIENT

# Add the requests url to attributes
_tracer.add_attribute_to_current_span(HTTP_URL, url)
# Add the requests host to attributes
_tracer.add_attribute_to_current_span(
HTTP_HOST, dest_url)

result = requests_func(url, *args, **kwargs)
# Add the requests method to attributes
_tracer.add_attribute_to_current_span(
HTTP_METHOD, requests_func.__name__.upper())

# Add the status code to attributes
# Add the requests path to attributes
_tracer.add_attribute_to_current_span(
HTTP_STATUS_CODE, str(result.status_code))
HTTP_PATH, path)

_tracer.end_span()
return result
# Add the requests url to attributes
_tracer.add_attribute_to_current_span(HTTP_URL, url)

try:
result = requests_func(url, *args, **kwargs)
except requests.Timeout:
_span.set_status(exceptions_status.TIMEOUT)
except requests.URLRequired:
_span.set_status(exceptions_status.INVALID_URL)
except Exception as e:
_span.set_status(exceptions_status.unknown(e))
else:
# Add the status code to attributes
_tracer.add_attribute_to_current_span(
HTTP_STATUS_CODE, result.status_code
)
_span.set_status(
utils.status_from_http_code(result.status_code)
)
return result
finally:
_tracer.end_span()

return call

Expand All @@ -113,10 +143,12 @@ def wrap_session_request(wrapped, instance, args, kwargs):
if utils.disable_tracing_hostname(dest_url, blacklist_hostnames):
return wrapped(*args, **kwargs)

path = parsed_url.path if parsed_url.path else '/'

_tracer = execution_context.get_opencensus_tracer()
_span = _tracer.start_span()

_span.name = '[requests]{}'.format(method)
_span.name = '{}'.format(path)
_span.span_kind = span_module.SpanKind.CLIENT

try:
Expand All @@ -127,14 +159,37 @@ def wrap_session_request(wrapped, instance, args, kwargs):
except Exception: # pragma: NO COVER
pass

# Add the requests url to attributes
_tracer.add_attribute_to_current_span(HTTP_URL, url)
# Add the requests host to attributes
_tracer.add_attribute_to_current_span(
HTTP_HOST, dest_url)

result = wrapped(*args, **kwargs)
# Add the requests method to attributes
_tracer.add_attribute_to_current_span(
HTTP_METHOD, method.upper())

# Add the status code to attributes
# Add the requests path to attributes
_tracer.add_attribute_to_current_span(
HTTP_STATUS_CODE, str(result.status_code))
HTTP_PATH, path)

# Add the requests url to attributes
_tracer.add_attribute_to_current_span(HTTP_URL, url)

_tracer.end_span()
return result
try:
result = wrapped(*args, **kwargs)
except requests.Timeout:
_span.set_status(exceptions_status.TIMEOUT)
Copy link

Choose a reason for hiding this comment

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

@reyang Isn't this except clause and the ones following going to eat the raised exception and returns None in case of any requests error? Unless I'm missing something this change is going to break many things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but wont requests return None in case it raises an Exception? If so, the returned value is the same

Copy link

Choose a reason for hiding this comment

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

Raising an exception should not "return" at all.

try:
  requests.get("http://not-a-host")
  unreacheable() #actually reached with this PR
except Exception:
  reached() # actually unreachable with this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What can be changed, and is a good idea actually, is to threat the Exception and raise it again, and let the user do whatever it wants. That's what you meant?

Copy link

Choose a reason for hiding this comment

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

This should be fixed by adding a raise statement in every except block.

Copy link

Choose a reason for hiding this comment

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

The main goal here is to keep this hook transparent and keep 1:1 interface as requests (Exceptions are part of that interface).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What can be changed, and is a good idea actually, is to threat the Exception and raise it again, and let the user do whatever it wants. That's what you meant?

So you agreed with this, right? Just to be clear. If so, I do agree with you and I will open a PR to change this.

Copy link

Choose a reason for hiding this comment

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

Yes 👍 !

Copy link
Contributor

Choose a reason for hiding this comment

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

@isra17 You're right, we've missed this. It is a bug.

except requests.URLRequired:
_span.set_status(exceptions_status.INVALID_URL)
except Exception as e:
_span.set_status(exceptions_status.unknown(e))
else:
# Add the status code to attributes
_tracer.add_attribute_to_current_span(
HTTP_STATUS_CODE, result.status_code
)
_span.set_status(
utils.status_from_http_code(result.status_code)
)
return result
finally:
_tracer.end_span()
Loading