botocore: switch setter for injecting headers into propagator #264
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Occasionally not all of the headers available to botocore are strings. It does allow this, using the `add_header` method rather than `__setitem__`. According to the docs for `botocore.compat.HTTPHeaders`: ``` If a parameter value contains non-ASCII characters it can be specified as a three-tuple of (charset, language, value), in which case it will be encoded according to RFC2231 rules. Otherwise it will be encoded using the utf-8 charset and a language of '' ``` Whereas using `__setitem__` does not do this conversion. Fixes open-telemetry#262
alertedsnake
requested review from
a team,
codeboten and
toumorokoshi
and removed request for
a team
alertedsnake
changed the title
toumorokoshi
suggested changes
Dec 19, 2020
| @@ -67,7 +67,7 @@ | |||
| def _patched_endpoint_prepare_request(wrapped, instance, args, kwargs): | |||
| request = args[0] | |||
| headers = request.headers | |||
| propagators.inject(type(headers).__setitem__, headers) | |||
| propagators.inject(type(headers).add_header, headers) | |||
There was a problem hiding this comment.
I don't have a lot of context on this, but we should probably add at least a test for the case you're running into.
From #262, it sounds like the most relevant part is the DyanamoDB API call. have you tried adding a mock DynamiDB api call (moto looks to support dynamodb)? It seems like a call that maps what you're executing in your service should trigger the error.
There was a problem hiding this comment.
Taking a closer look, the part I'm a little unclear on is, as you mentioned, what is actually causing this. I have a feeling something was injected that should not have been by the propagator. I'll comment more on the issue.
|
Closing this, it's not the root cause of #262 and so doesn't seem necessary. |
nstawski
pushed a commit
to nstawski/ns-opentelemetry-python-contrib
that referenced
this pull request
May 10, 2023
…ry#264) Adding the ext.dbapi and ext.mysql package.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Occasionally not all of the headers available to botocore are strings - I've noticed this when a call is being made with an active span passed from a different application (specifically via gRPC). Botocore does allow this, using the
add_headermethod rather than__setitem__. According to the docs forbotocore.compat.HTTPHeaders:Whereas using
__setitem__does not do this conversion.I'm not convinced this is the right place to fix this, however - I've only encountered it in the situation described in #262, and haven't been able to find what would be passing in a non-string header in the first place. I'll be happy to dig more if anyone has a better suggestion, but as far as I can tell, this is also an innocuous change.
One question I have though is about how I could test this - I don't know how to simulate (in the botocore tests) having an active trace passed through from another application, so advice on this would be welcome.
Fixes #262
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
All existing tests still pass, and I've been working with the situation described in #262
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.