-
Notifications
You must be signed in to change notification settings - Fork 144
Support fot Aiohttp Client tracing, aiohttp> 3.0.0 #42
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
Conversation
New `aws_xray_trace_config()` function to retrieve a `aiohttp.TraceConfig` object ready to be used in any `ClientSession`, once the Aiohttp Client session is instantiated with this trace config all sampled HTTP calls will be traced as subsegements and the data properly uploaded.
aws_xray_sdk/ext/aiohttp/client.py
Outdated
xray_recorder.end_subsegment() | ||
|
||
|
||
def aws_xray_trace_config(name=None, namespace=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Third party library support should have non-None default value as the library has the context of what is captured. For example, boto
defaults the namespace to aws
and requests
defaults the namespace to remote
. The aiohttp client is used for making outbound calls so it should be default to remote
with the option to be set to something else (although I cannot think of any use case that anyone needs to do that).
Setting namespace to remote
makes sure the service graph renders a node for that endpoint. The X-Ray backend only renders a node when 1. that node actively sends segment or 2. the namespace of the subsegment representing that node is aws
or remote
. To be more specific, if you use the client to call a service B
which you don't own, then B
is unlikely to generate any trace data. If this call is marked as remote
, you can see B
on your service graph.
The only case where you might want to mark an out-going http call subsegment to local
depends on the http client implementation. If the client thrown a ClientError
when the call cannot even go out due to connection timeout, DNS lookup fails, invalid input parameters etc, you might want to flip it back to local
as the error has nothing to do with the downstream service. This is most time a bonus as a lot of libraries doesn't let you easily categorize if the error is from local or from downstream (easy means a proper public API).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know, so better remove it as a param having it as a constant using always remote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would purpose to change to def aws_xray_trace_config(name=None, namespace='remote'):
.
tox.ini
Outdated
|
||
setenv = | ||
DJANGO_SETTINGS_MODULE = tests.ext.django.app.settings | ||
|
||
[testenv:py36-aiohttp3] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the client 3.x support requires Py3.6+?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nops but tox uses 3.5.2 that is not compatible, aiohttp needs at least 3.5.3. Any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if I understand this correctly. Tox will use pre-installed interpreter and skip non-exist ones. If your system installs py3.5.2 it will pick that up. If your system doesn't have py3.5 installed it will just skip py35 testing. I'd prefer to add py35 here so it is not mistakenly considered that this new feature only works with py3.6+.
tests/ext/aiohttp/test_client.py
Outdated
assert subsegment.fault | ||
|
||
exception = subsegment.cause['exceptions'][0] | ||
assert exception.type == 'ClientConnectorError' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where a local
namespace makes sense. But it is optional to optimize end_subsegment_with_exception
as it is not realistic to categorize every single error thrown by the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nops but tox uses 3.5.2 that is not compatible, aiohttp needs at least 3.5.3. Any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forget about last comment. Its kinda weir that the place where the error comes from is handled by the value of the namespace.
We can try to mark the most known exceptions as local and others as remote. In any case my doubt here is that taking into accout that the namespace is used to create the subsegment and the error is catched after the creation, can I modify the namespqce once is created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the other way around I believe: the namespace might need to be flipped back to remote
depending on where the error comes from.
In this case the default captured subsegments should be remote
as previously discussed. You can flip it back to local
especially an error like this is thrown. After all non such endpoint could possibly exist so there won't be any downstream.
Two more things: one is the change log 'unreleased' section needs to be updated. The other one is the |
@haotianw465 all change that you asked applied, please take a look. Regarding the |
It all looks good. Just one question left. There is |
I didn't merge it because I didn't find a way to make it in a clean way, not because I wanted the tox passed for my environment. To sum up, I'm a newbie with tox and after hitting my head on the same wall many times I gave up. So, if you have a better proposal for the tox .. I will be really glad to hear you :) |
This is not a blocking issue. We will potentially do some tweaks to |
@haotianw465 any chance to have a new release? I think that there are some interesting features in master that users will appreciate it, among them us. |
Yes. I cannot provide a timeline but I am definitely trying to get the new changes out as soon as possible. |
@pfreixes just letting you know 0.97 with all latest changes has been released. |
many thanks ! |
New
aws_xray_trace_config()
function to retrieve aaiohttp.TraceConfig
object ready to be used in anyClientSession
, once the Aiohttp Client session is instantiated with this trace config all sampled HTTP calls will be traced as subsegments and the data properly uploaded.Usage example: