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

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 4 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
100 changes: 83 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 @@ -23,6 +23,7 @@
from opencensus.trace import attributes_helper
from opencensus.trace import execution_context
from opencensus.trace import span as span_module
from opencensus.trace import status as status_module
from opencensus.trace import utils

log = logging.getLogger(__name__)
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,53 @@ 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(
status_module.Status(1, message='request timed out')
victoraugustolls marked this conversation as resolved.
Show resolved Hide resolved
)
except requests.URLRequired:
_span.set_status(
status_module.Status(3, message='invalid URL')
)
except Exception as e:
_span.set_status(
status_module.Status(2, message=str(e))
)
else:
# Add the status code to attributes
_tracer.add_attribute_to_current_span(
HTTP_STATUS_CODE, str(result.status_code)
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the spec, status_code is an int64. /~https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/HTTP.md#mapping-from-http-status-codes-to-trace-status-codes

This again seems to be a spec bug in the test-cases section, where status_code is put as a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is better to leave as int64, following specs. But open for discussion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the spec, status_code is an int64. /~https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/HTTP.md#mapping-from-http-status-codes-to-trace-status-codes

Right now AzureExporter for tracer is broken because Flask, Django and Pyramid integrations set status_code as string, and azures expect int, we can typecast on azure or change the other integrations, which way is preferred?

Copy link
Contributor

Choose a reason for hiding this comment

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

/~https://github.com/census-instrumentation/opencensus-python/blame/master/contrib/opencensus-ext-azure/opencensus/ext/azure/trace_exporter/__init__.py#L86

My understanding is that we'll still get the right status_code, but the success flag will be wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that we'll still get the right status_code, but the success flag will be wrong.

Yes, and with this we have both options described above. I can take any of them, I prefer to put the status_code as int, it makes more sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you that we should use int. Converting int to string and then parse it to int sounds like crazy :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will open a new PR to fix this into Flask, Django and Pyramid integrations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did it here.

)
_span.set_status(
utils.status_from_http_code(result.status_code)
)
return result
finally:
_tracer.end_span()

return call

Expand All @@ -113,10 +149,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 +165,42 @@ 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(
status_module.Status(1, message='request timed out')
)
except requests.URLRequired:
_span.set_status(
status_module.Status(3, message='invalid URL')
)
except Exception as e:
_span.set_status(
status_module.Status(2, message=str(e))
)
else:
# Add the status code to attributes
_tracer.add_attribute_to_current_span(
HTTP_STATUS_CODE, str(result.status_code))
_span.set_status(
utils.status_from_http_code(result.status_code)
)
return result
finally:
_tracer.end_span()
Loading