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

Fix RequestsInstrumentor for custom transport adapters #562

Merged
merged 6 commits into from
Jul 14, 2021
Merged

Fix RequestsInstrumentor for custom transport adapters #562

merged 6 commits into from
Jul 14, 2021

Conversation

mariojonke
Copy link
Contributor

@mariojonke mariojonke commented Jul 2, 2021 •
edited
Loading

The RequestsInstrumentor contained some dead code from an early metrics implementation that in certain situations failed with an AttributeError. The AttributeError happened when trying to determine the HTTP flavor from the raw.version attribute on the returned Response object.
By default, the raw attribute is an urllib3 response that has the version attribute. However, requests allows mounting custom transport adapters for certain URLs.
A custom adapter can return a Response where the raw object isn't an urllib3 response and the version attribute might not be present. Thus, the RequestsInstrumentor could run into an AttributeError.

This change removes the dead metrics code.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Additional unit test to verify that the RequestsInstrumentor works with custom transport adapters.

Does This PR Require a Core Repo Change?

  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added

@mariojonke
Fix RequestsInstrumentor for custom transport adapters
0e7fc3d 
remove dead/leftover code from an early metrics implementation which
tried to access the raw.version attribute on the response object.
The 'version' attribute might not be present in every case, especially
when custom transport adapters are used.
@mariojonke mariojonke requested review from a team, ocelotl and lzchen and removed request for a team July 2, 2021 07:21
codeboten
codeboten reviewed Jul 9, 2021
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Change looks good, I just have one question before approving.

...pentelemetry-instrumentation-requests/src/opentelemetry/instrumentation/requests/__init__.py
@@ -131,10 +131,6 @@ def _instrumented_requests_call(

url = remove_url_credentials(url)

labels = {}
labels[SpanAttributes.HTTP_METHOD] = method
labels[SpanAttributes.HTTP_URL] = url
Copy link
Contributor

Choose a reason for hiding this comment

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

These changes don't seem directly related to the description of this change, were these attributes superfluous?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is the dead code @mariojonke is talking about in the PR description.

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, the labels are a leftover from the old metrics implementation. Since all the metrics related code got removed, the labels are unused.

@srikanthccv
Copy link
Member

Do client semantic conventions allow http.falvor? I think we could do something like hasattr and add the span attribute instead of entirely removing it? WDYT?

@mariojonke
Copy link
Contributor Author

Semantic conventiosn for http.flavor exist in the spec for server and client spans.
However, parsing the flavor from the response might be unreliable since

  1. a custom response might also have a version attribute
  2. even if it is a urllib3 HTTPResposnse the version attribute is provided only from yet another response object. Usually it is a http.client.HTTPResponse but i'm not sure if this is always the case.
    So the format of the version attribute might actually be different from what is expected.

Also, I think the removed code for parsing the version appears to be incorrect. In the HTTPResponse docs the version is defined as 10 for HTTP/1.0 and 11 for HTTP/1.1. However, for 10 the http.flavor attribute would be parsed to 1.1 due to how the version string is sliced.

@codeboten
Copy link
Contributor

@lonewolf3739 please approve if your comments have been addressed, thanks!

@codeboten codeboten merged commit 2ccf120 into open-telemetry:main Jul 14, 2021
@mariojonke mariojonke deleted the fix-requests-custom-adapters branch October 8, 2021 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanthccv srikanthccv srikanthccv approved these changes

@ocelotl ocelotl ocelotl approved these changes

@codeboten codeboten codeboten approved these changes

@lzchen lzchen Awaiting requested review from lzchen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mariojonke @srikanthccv @codeboten @ocelotl