Change client tracing signatures

[asvetlov]

Now tracing signal signatures look like

async def on_request_start(session, trace_config_ctx, method, host, port, headers): ...
async def on_send_connection_queued_start(session, trace_config_ctx): ...

The problem is the approach is has no space for future extensions.
Most likely we will want to add a connection info to on_send_connection_queued_start but it is impossible to do without breaking all existing signal handlers: sending additional connection parameter cannot be accepted.

Sure, we can enforce users to write handlers with mandatory *args and **kwargs like

async def on_send_connection_queued_start(session, trace_config_ctx, *args, **kwargs): ...

but I expect that users will constantly forgot these extra arguments.

As a solution we can change signal signature for all signals to async def on_sig(session, ctx, data): ... where ctx is trace_config_ctx and data is a structure with all signal parameters: url, host, port, headers, connection etc.
It allows to add new param to data in backward compatible manner.

Next question is the data type.
I see several options:

1. Plain dict with keys like host and connection. It works but looks too old fashion, everybody prefer attributes to string keys.
2. SimpleNamespace. data.url looks better than data['url']
3. Custom data classes. Honestly it is even better than SimpleNamespace at least from documentation perspective: we will need to document every used data structure explicitly.
4. attrs classes. The library brings extra dependency (I don't care, aiohttp has several deps already) but provides neat helpers. Using data classes from Python 3.7 is even better but we should support Python 3.5/3.6 for long time.

Trace class already has explicit methods for sending every signal, constructing a data class on call is easy and straightforward.

@pfreixes and others, what do you think about?

[pfreixes]

Definitly I wil go for something explicit that incorporates documentation. What about thr idea of custom data classes but based on simple namedtuples?

[asvetlov]

I think deriving from SimpleNamespace makes no sense.

[pfreixes]

so, lets try it with derivating namedtuples? or you are keen on make it even more explicit, that IMHO might be to much verbose.

[asvetlov]

Do you have objections against attrs library?

[pfreixes]

I didnt know about attrts, looks great. But just for simplicity and consistence - current aiohttp code usage - and taking into account that the use case is just a need of having a explicit way to declare our data params.

Do you believe that right now the features that this library gives are gonna make a difference from the begining?

[pfreixes]

That's true that after the acceptation of the data classes PEP, at some point this usage of namedtuples could become old-fashioned. And the usage of the attrs package makes a step in the right direction.

But for the sake of consistency don't you believe that we must stick to just one pattern in Aiohttp?

[asvetlov]

Replacing namedtuples with attrs is my old big secret plan, tracing classes is a good point to start implementing it :)

[pfreixes]

Oks, let's go for attr. Any volunteer @aio-libs/aiohttp-committers ?

[pfreixes]

Just a POC [1] that should be enough to have some feedback from you @asvetlov and see if this is what are you expecting after moving to attrs

[1] pfreixes@00061ce

[asvetlov]

Please wait for #2690 landing.
Also providing a type of attributes improves code readability.

[pfreixes]

Working on that ... just to avoid race conditions.

[pfreixes]

it can be closed

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new