aiohttp instrumentation: Function create_trace_config has no span_name argument.

[Olegt0rr]

Docs usage example

Ref: https://opentelemetry-python-contrib.readthedocs.io/en/latest/instrumentation/aiohttp_client/aiohttp_client.html#usage

import aiohttp
from opentelemetry.instrumentation.aiohttp_client import (
    create_trace_config,
    url_path_span_name
)
import yarl

def strip_query_params(url: yarl.URL) -> str:
    return str(url.with_query(None))

async with aiohttp.ClientSession(trace_configs=[create_trace_config(
        # Remove all query params from the URL attribute on the span.
        url_filter=strip_query_params,
        # Use the URL's path as the span name.
        span_name=url_path_span_name
)]) as session:
    async with session.get(url) as response:
        await response.text()

Bug

Function create_trace_config has no span_name argument.

def create_trace_config(
    url_filter: _UrlFilterT = None,
    request_hook: _RequestHookT = None,
    response_hook: _ResponseHookT = None,
    tracer_provider: TracerProvider = None,
) -> aiohttp.TraceConfig:
    ...

[Olegt0rr]

Also looks weird that current solution starts span with name f"HTTP {http_method}" and then prompts to change it through request_hook callback

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-aiohttp-client/src/opentelemetry/instrumentation/aiohttp_client/__init__.py

Lines 200 to 210 in d01efe5

	http_method = params.method.upper()
	request_span_name = f"HTTP {http_method}"
	trace_config_ctx.span = trace_config_ctx.tracer.start_span(
	request_span_name,
	kind=SpanKind.CLIENT,
	)
	if callable(request_hook):
	request_hook(trace_config_ctx.span, params)

It would be nice to add span_name (as it described in the docs).

def create_trace_config(
    ...
    span_name: Optional[str] = None,
) -> aiohttp.TraceConfig:
    ...
    request_span_name = f"HTTP {http_method}" if span_name is None else span_name

[owais]

Proposed patch looks good to me. Happy to merge a PR if one is sent :)

[lzchen]

@owais
I thought we were trying to move away from using a span name callback and utilize the request/response hooks instead?

Like here: #409

[owais]

@lzchen This isn't a callback and since it is documented, it looked like a mistake but you're right, we already support request/response hooks which solve this problem. May be we should just update the docs instead.

[Olegt0rr]

Okay, I'll revert changes and update the docs.

Should I remove url_path_span_name since it's not usable anymore?

[lzchen]

@Olegt0rr

Yes it doesn't seem like url_path_span_name is used so feel free to remove it. I think updating the docs to remove span_name is enough for this issue.

[Olegt0rr]

@lzchen, ready :)