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.

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

Use instanceof to check if responses are valid Response objects #273

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

ThePumpingLemma
Copy link
Contributor

@ThePumpingLemma ThePumpingLemma commented Dec 22, 2020

There are some exception types that set the response attribute to a
non-Response type, e.g. "AttributeError: 'dict' object has no attribute
'status_code'"

Description

At $WORK, we have an integration with a vendor (i.e. POST calls via requests) that will fail once-in-a-while with an exception where the response attribute on the exception is a dictionary instead of a Response object. Because the instrumentation doesn't have a type check, it tries to get the status_code attribute from the dictionary and fails with an AttributeError. Since it swallows the exception thrown from requests, we can't easily debug it (vendor issue is not easily reproducible). This change will allow the instrumentation to work in this edge case.

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Added unit tests

Does This PR Require a Core Repo Change?

  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

@ThePumpingLemma ThePumpingLemma requested review from a team, owais and hectorhdzg and removed request for a team December 22, 2020 23:41
@ThePumpingLemma ThePumpingLemma force-pushed the check-response-type branch 2 times, most recently from b800adc to 9670a95 Compare December 23, 2020 01:38
@lzchen
Copy link
Contributor

lzchen commented Jan 7, 2021

@ThePumpingLemma
If you can fix the conflicts we can merge this.

There are some exception types that set the response attribute to a
non-Response type, e.g. "AttributeError: 'dict' object has no attribute
'status_code'"
@ThePumpingLemma
Copy link
Contributor Author

@ThePumpingLemma
If you can fix the conflicts we can merge this.

Fixed.

@lzchen lzchen merged commit cb01a6b into open-telemetry:master Jan 8, 2021
@ThePumpingLemma ThePumpingLemma deleted the check-response-type branch January 8, 2023 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants