Requests library spec fidelity

[victoraugustolls]

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.

[reyang]

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.

[victoraugustolls]

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

[victoraugustolls]

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?

[reyang]

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.

[victoraugustolls]

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.

[reyang]

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

[victoraugustolls]

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

[victoraugustolls]

Did it here.

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

[reyang]

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

[c24t]

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

[isra17]

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

[victoraugustolls]

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

[isra17]

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

[victoraugustolls]

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?

[isra17]

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

[isra17]

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

[victoraugustolls]

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]

Yes 👍 !

[reyang]

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

[isra17]

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