Skip to content

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

Merged
merged 7 commits into from
Mar 20, 2018

Conversation

pfreixes
Copy link
Contributor

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 subsegments and the data properly uploaded.

Usage example:

from aws_xray_sdk.ext.aiohttp.client import aws_xray_trace_config

trace_config = aws_xray_trace_config()
async with ClientSession(loop=loop, trace_configs=[trace_config]) as session:
     async with session.get(url) as resp
        await resp.read()

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.
xray_recorder.end_subsegment()


def aws_xray_trace_config(name=None, namespace=None):
Copy link
Contributor

@haotianw465 haotianw465 Mar 19, 2018

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).

Copy link
Contributor Author

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?

Copy link
Contributor

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]
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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+.

assert subsegment.fault

exception = subsegment.cause['exceptions'][0]
assert exception.type == 'ClientConnectorError'
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

@haotianw465
Copy link
Contributor

haotianw465 commented Mar 19, 2018

Two more things: one is the change log 'unreleased' section needs to be updated. The other one is the quick start on README has content about adding aiohttp middleware. It would be good for the title changed to working with aiohttp and update the content to cover both middleware and client.

@pfreixes
Copy link
Contributor Author

@haotianw465 all change that you asked applied, please take a look.

Regarding the tox issue I've added a new environment for the Python 3.5 that basically replicates the Python 3.6, I'm sure that there is an alternative way to make so that should follow the tox principles, but I didn't manage to make it. So any advice to improve the current implementation will be welcomed.

@haotianw465
Copy link
Contributor

It all looks good. Just one question left.

There is skip_missing_interpreters = True in tox.ini file. What happens if you merge py35 and py36 in one environment? Does it fail on your end? If your system doesn't have py35 installed or have >=3.5.3 installed it should be fine I believe.

@pfreixes
Copy link
Contributor Author

pfreixes commented Mar 20, 2018

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 :)

@haotianw465
Copy link
Contributor

This is not a blocking issue. We will potentially do some tweaks to tox.ini to make it better.

@haotianw465 haotianw465 merged commit 9ff8539 into aws:master Mar 20, 2018
@pfreixes
Copy link
Contributor Author

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

@haotianw465
Copy link
Contributor

Yes. I cannot provide a timeline but I am definitely trying to get the new changes out as soon as possible.

@haotianw465
Copy link
Contributor

@pfreixes just letting you know 0.97 with all latest changes has been released.

@pfreixes
Copy link
Contributor Author

pfreixes commented Apr 3, 2018

many thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants