requests headers none argument fix #708

HiveTraum · 2020-05-19T11:43:59Z

Hi everyone!

My integration stuck with this little error. If to headers argument explicitly passed None value - it raises exception

codeboten · 2020-05-19T14:42:51Z

Hey @HiveTraum, thanks for the fix! You'll need to sign the CLA before it can be accepted.

HiveTraum · 2020-05-19T14:47:46Z

I signed it

HiveTraum · 2020-05-19T14:49:03Z

@codeboten , success, all checks passed

ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py

codeboten

Thanks for the contribution @HiveTraum . Just one question I'd like to get answered before approving.

codeboten · 2020-05-27T16:03:15Z

ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py

+            headers = kwargs.get("headers", {}) or {}
            propagators.inject(type(headers).__setitem__, headers)
+            kwargs["headers"] = headers


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)

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

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

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

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.

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

@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

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 ^

Thanks for following up on this. Approving

toumorokoshi

same changes that @codeboten is requesting. Happy to approve once that's addressed.

Thanks for the PR!

toumorokoshi · 2020-05-28T04:30:53Z

ext/opentelemetry-ext-requests/tests/test_requests_integration.py

+
+    def test_if_headers_equals_none(self):
+        RequestsInstrumentor().uninstrument()
+        RequestsInstrumentor().instrument(tracer_provider=TracerProvider())


what purpose does clearing the instrumentor serve? by default the instrumentor is turned on, so I imagine the error would surface regardless.

agreed with you, removed this

deleted this redudant instrumentation

toumorokoshi · 2020-05-28T04:34:09Z

ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py

+            headers = kwargs.get("headers", {}) or {}
            propagators.inject(type(headers).__setitem__, headers)
+            kwargs["headers"] = headers


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

toumorokoshi

@HiveTraum I see you've been making changes since the last review, which is great!

But I think the core of the original review is not yet addressed. see my new comment. That's the conversation we need to resolve before merging.

Thanks!

toumorokoshi · 2020-06-05T17:28:06Z

ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py

+            headers = kwargs.get("headers", {}) or {}
            propagators.inject(type(headers).__setitem__, headers)
+            kwargs["headers"] = headers


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

linux-foundation-easycla · 2020-06-08T06:39:46Z

The committers are authorized under a signed CLA.

✅ HiveTraum (3212d5e, 8e51567, ef2d129, 3826a2c, 36583a5, f330aca, 95d132a)
✅ alrex (b872b91, dc02ae5)

toumorokoshi

Thanks for clarifying!

toumorokoshi · 2020-06-09T03:40:14Z

ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py

+            headers = kwargs.get("headers", {}) or {}
            propagators.inject(type(headers).__setitem__, headers)
+            kwargs["headers"] = headers


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 ^

* chore: simplify and speed up trace context parsing

requests headers none argument fix

3212d5e

HiveTraum requested a review from a team May 19, 2020 11:43

clean up, lint fix

8e51567

codeboten reviewed May 19, 2020

View reviewed changes

ext/opentelemetry-ext-requests/src/opentelemetry/ext/requests/__init__.py Show resolved Hide resolved

codeboten suggested changes May 19, 2020

View reviewed changes

HiveTraum requested a review from codeboten May 21, 2020 09:12

codeboten reviewed May 27, 2020

View reviewed changes

toumorokoshi suggested changes May 28, 2020

View reviewed changes

HiveTraum and others added 5 commits May 28, 2020 10:36

removed redudant requests instrumentor

ef2d129

lint fix

3826a2c

instrumenting returned in requests tests

36583a5

Merge branch 'master' into fix/requests-none-header-argument

b872b91

test cleanup

f330aca

HiveTraum requested review from codeboten and toumorokoshi June 2, 2020 15:07

toumorokoshi suggested changes Jun 5, 2020

View reviewed changes

Merge branch 'master' into fix/requests-none-header-argument

95d132a

toumorokoshi approved these changes Jun 9, 2020

View reviewed changes

codeboten approved these changes Jun 9, 2020

View reviewed changes

Merge branch 'master' into fix/requests-none-header-argument

dc02ae5

codeboten merged commit 93194e1 into open-telemetry:master Jun 9, 2020

codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Jun 11, 2020

fix: requests headers none argument fix (open-telemetry#708)

a514d21

srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020

Simplify and speed up trace context parsing (open-telemetry#708)

8496fb2

* chore: simplify and speed up trace context parsing

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

requests headers none argument fix #708

requests headers none argument fix #708

HiveTraum commented May 19, 2020

codeboten commented May 19, 2020

HiveTraum commented May 19, 2020

HiveTraum commented May 19, 2020

codeboten left a comment

codeboten May 27, 2020 •

edited

Loading

HiveTraum May 28, 2020

toumorokoshi May 28, 2020

lzchen Jun 2, 2020

codeboten Jun 5, 2020

toumorokoshi Jun 5, 2020

HiveTraum Jun 8, 2020 •

edited

Loading

toumorokoshi Jun 9, 2020

codeboten Jun 9, 2020

toumorokoshi left a comment

toumorokoshi May 28, 2020

HiveTraum May 28, 2020

HiveTraum Jun 2, 2020

toumorokoshi May 28, 2020

toumorokoshi left a comment

toumorokoshi Jun 5, 2020

linux-foundation-easycla bot commented Jun 8, 2020 •

edited

Loading

toumorokoshi left a comment

toumorokoshi Jun 9, 2020

requests headers none argument fix #708

requests headers none argument fix #708

Conversation

HiveTraum commented May 19, 2020

codeboten commented May 19, 2020

HiveTraum commented May 19, 2020

HiveTraum commented May 19, 2020

codeboten left a comment

Choose a reason for hiding this comment

codeboten May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HiveTraum Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Jun 8, 2020 • edited Loading

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten May 27, 2020 •

edited

Loading

HiveTraum Jun 8, 2020 •

edited

Loading

linux-foundation-easycla bot commented Jun 8, 2020 •

edited

Loading