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

requests headers none argument fix #708

Merged
merged 9 commits into from
Jun 9, 2020
Merged

requests headers none argument fix #708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3212d5e
requests headers none argument fix
HiveTraum May 19, 2020
8e51567
clean up, lint fix
HiveTraum May 19, 2020
ef2d129
removed redudant requests instrumentor
HiveTraum May 28, 2020
3826a2c
lint fix
HiveTraum May 28, 2020
36583a5
instrumenting returned in requests tests
HiveTraum May 28, 2020
b872b91
Merge branch 'master' into fix/requests-none-header-argument
May 28, 2020
f330aca
test cleanup
HiveTraum Jun 2, 2020
95d132a
Merge branch 'master' into fix/requests-none-header-argument
HiveTraum Jun 8, 2020
dc02ae5
Merge branch 'master' into fix/requests-none-header-argument
Jun 9, 2020
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
3 changes: 2 additions & 1 deletion ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,9 @@ def instrumented_request(self, method, url, *args, **kwargs):
span.set_attribute("http.method", method.upper())
span.set_attribute("http.url", url)

headers = kwargs.setdefault("headers", {})
headers = kwargs.get("headers", {}) or {}
toumorokoshi marked this conversation as resolved.
Show resolved Hide resolved
propagators.inject(type(headers).__setitem__, headers)
kwargs["headers"] = headers
Comment on lines +99 to +101
Copy link
Contributor

@codeboten codeboten May 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry it took me this long to get back to you @HiveTraum, what I meant by my question was could this solve the problem instead of setting the headers attribute:

Suggested change
headers = kwargs.get("headers", {}) or {}
propagators.inject(type(headers).__setitem__, headers)
kwargs["headers"] = headers
headers = kwargs.get("headers", {})
if headers is not None:
propagators.inject(type(headers).__setitem__, headers)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case request will not be traced, is that what we want? then it will be implicit

Copy link
Member

Choose a reason for hiding this comment

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

agreed that this approach should be taken.

Regarding the need for this PR at all: I guess requests does accept a None as headers. But looking at the API documentation, I don't see None as a valid value to pass in: https://requests.readthedocs.io/en/master/api/#requests.request

Copy link
Contributor

Choose a reason for hiding this comment

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

@codeboten
I might be missing something here, if the header is None, we don't propagate the trace context but we still create the span? Would the trace just end there?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct @lzchen, no trace would be propagated here. I guess this is a bit of a strange case, but if as a user I explicitly set headers to None when sending a request, I'm not sure I would expect an instrumentation library to add a header regardless of my initial setting.

Copy link
Member

Choose a reason for hiding this comment

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

in this case request will not be traced, is that what we want? then it will be implicit

@HiveTraum to clarify: propagators should not inject anything into headers if no such headers exist. At least, that's the standard that's used for every propagation scheme I know.

I think in this case it's fine, and I would argue more elegant than what's there now. Adding this skips the need to replace the headers entirely in the kwargs on line #97

Copy link
Contributor Author

@HiveTraum HiveTraum Jun 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@dataclass
class OrgStructureServiceEndpoint:
    url: str
    method: str = 'GET'
    headers: dict = None
    domain: str = None
    params: dict = None

    def send(self):
        url = f"{self.domain or settings.ORGSTRUCT_URL}/{self.url}"
        try:
            response = requests.request(
                method=self.method, url=url, headers=self.headers,
                params=self.params, timeout=settings.REQUEST_TIMEOUT)
        except requests.exceptions.Timeout:
            raise APIException("Не удалось связаться с сервисом Оргструктуры")

I have such example which shows implicit scenario when user can pass None headers implicitly. As user i want to see this request in traces.

I think that if user wants to send request without headers at all - suppress_instrumentation must be used and headers=None

Copy link
Member

Choose a reason for hiding this comment

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

Okay, got it. Yeah I see that case now. Looking at the requests source code, it looks like things are handled in a very similar way (default to an empty dict):

    def __init__(self,
            method=None, url=None, headers=None, files=None, data=None,
            params=None, auth=None, cookies=None, hooks=None, json=None):

        # Default empty dicts for dict params.
        data = [] if data is None else data
        files = [] if files is None else files
        headers = {} if headers is None else headers
        params = {} if params is None else params
        hooks = {} if hooks is None else hooks

So yes, this make sense. I'll switch to approved. Thanks!

@codeboten so you can see this too ^

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for following up on this. Approving


try:
result = wrapped(
Expand Down
4 changes: 4 additions & 0 deletions ext/opentelemetry-ext-requests/tests/test_requests_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,10 @@ def test_custom_tracer_provider(self):

self.assertIs(span.resource, resource)

def test_if_headers_equals_none(self):
result = requests.get(self.URL, headers=None)
self.assertEqual(result.text, "Hello!")

@mock.patch("requests.Session.send", side_effect=requests.RequestException)
def test_requests_exception_without_response(self, *_, **__):

Expand Down