Requests library spec fidelity by victoraugustolls · Pull Request #746 · census-instrumentation/opencensus-python

victoraugustolls · 2019-08-05T16:59:17Z

Updated requests integration for better fidelity, adding attributes:

http host
http method
http path

Updated span status created by requests integration.
Updated span name to follow specs.

Added http status code to grpc status code mapping as an utility.

contrib/opencensus-ext-requests/opencensus/ext/requests/trace.py

reyang · 2019-08-06T04:45:02Z

contrib/opencensus-ext-requests/opencensus/ext/requests/trace.py

+        else:
+            # Add the status code to attributes
+            _tracer.add_attribute_to_current_span(
+                HTTP_STATUS_CODE, str(result.status_code)


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.

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

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?

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.

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.

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

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

Did it here.

opencensus/trace/utils.py

reyang

Looks good in general. I've left a few comments.
I'm a bit surprised to see the bugs in specs. Wish we will do a better job in OpenTelemetry.

opencensus/trace/exceptions_status.py

reyang

LGTM.
Thanks @victoraugustolls! Let's wait for one day before merge, see if @c24t has comments.

c24t · 2019-08-06T19:36:23Z

opencensus/trace/exceptions_status.py

+
+CANCELLED = Status(code_pb2.CANCELLED)
+INVALID_URL = Status(code_pb2.INVALID_ARGUMENT, message='invalid URL')
+TIMEOUT = Status(code_pb2.DEADLINE_EXCEEDED, message='request timed out')


@victoraugustolls there's an ongoing discussion about using grpc error codes in OpenTelemetry, if you've got an opinion here you may want to tune into the SIG meeting or follow the OT specs repo.

c24t

Code and tests look excellent, and this is a clear improvement.

leameow · 2019-08-14T14:35:19Z

contrib/opencensus-ext-requests/opencensus/ext/requests/trace.py

+    try:
+        result = wrapped(*args, **kwargs)
+    except requests.Timeout:
+        _span.set_status(exceptions_status.TIMEOUT)


@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.

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

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

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?

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

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

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.

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

leameow · 2019-08-14T14:52:21Z

contrib/opencensus-ext-requests/tests/test_requests_trace.py

+        url = 'http://localhost:8080/test'
+
+        with patch, patch_thread:
+            wrapped(url)


To be clearer, given mock_func raise an exception, this test case should fail gith here and never go further. The test should actually assert the requests.TooManyRedirects exception is raised from wrapped(url).

victoraugustolls added 2 commits August 5, 2019 13:54

Added util for mapping http status code to grpc status code

b24abd9

Updated requests integration to follow specs

73ba2fe

victoraugustolls requested review from a team, c24t, reyang and songy23 as code owners August 5, 2019 16:59

googlebot added the cla: yes label Aug 5, 2019

Updated changelogs

e1d8152

victoraugustolls mentioned this pull request Aug 5, 2019

SD exporter and set_status release #750

Merged

Merge branch 'master' into feature/requests-specs

3c6882c

reyang reviewed Aug 6, 2019

View reviewed changes

contrib/opencensus-ext-requests/opencensus/ext/requests/trace.py Outdated Show resolved Hide resolved

reyang reviewed Aug 6, 2019

View reviewed changes

opencensus/trace/utils.py Outdated Show resolved Hide resolved

reyang approved these changes Aug 6, 2019

View reviewed changes

reyang mentioned this pull request Aug 6, 2019

Update Flask middleware to not use full URL with query params as a span name #725

Closed

1 task

victoraugustolls added 3 commits August 6, 2019 12:48

Added common exception status

7e2c3e3

Simplified http status code to status mapping

524dd23

Now using exceptions mappings

d5ed389

reyang reviewed Aug 6, 2019

View reviewed changes

opencensus/trace/exceptions_status.py Outdated Show resolved Hide resolved

reyang approved these changes Aug 6, 2019

View reviewed changes

victoraugustolls added 2 commits August 6, 2019 13:38

Removed DNS_RESOLUTION_FAILED mapping

b687467

Fixed exceptions status tests

d62efcc

c24t reviewed Aug 6, 2019

View reviewed changes

c24t approved these changes Aug 6, 2019

View reviewed changes

victoraugustolls added 2 commits August 6, 2019 16:55

Merge branch 'master' into feature/requests-specs

09a060e

Merge branch 'master' into feature/requests-specs

2823238

reyang merged commit ab47474 into census-instrumentation:master Aug 6, 2019

victoraugustolls deleted the feature/requests-specs branch August 6, 2019 22:38

reyang mentioned this pull request Aug 7, 2019

Update codeowners file #756

Merged

leameow reviewed Aug 14, 2019

View reviewed changes

Conversation

victoraugustolls commented Aug 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

reyang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c24t left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

victoraugustolls commented Aug 5, 2019 •

edited

Loading