WSGI request spans don't have both http.target and http.url attributes

[alexmojaki]

Steps to reproduce

Example code:

from flask import Flask
from opentelemetry.instrumentation.flask import FlaskInstrumentor
from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.export import ConsoleSpanExporter, SimpleSpanProcessor
from opentelemetry.semconv.trace import SpanAttributes

tracer_provider = TracerProvider()
processor = SimpleSpanProcessor(
    ConsoleSpanExporter(
        formatter=lambda span: str(
            (
                span.attributes.get(SpanAttributes.HTTP_TARGET),
                span.attributes.get(SpanAttributes.HTTP_URL),
            )
        )
    )
)
tracer_provider.add_span_processor(processor)

app = Flask(__name__)
FlaskInstrumentor().instrument_app(app, tracer_provider=tracer_provider)


@app.route('/hello')
def hello():
    return 'hello'


app.run()

Then visit http://localhost:5000/hello

What is the expected behavior?

('/hello', 'http://localhost:5000/hello')

What is the actual behavior?

('/hello', None)

Additional context

The current code looks like this:

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

Lines 320 to 325 in 47caeab

	if target is not None:
	result[SpanAttributes.HTTP_TARGET] = target
	else:
	result[SpanAttributes.HTTP_URL] = remove_url_credentials(
	wsgiref_util.request_uri(environ)
	)

The first commit of the file shows essentially the same:

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py

Lines 108 to 111 in d19b464

	if target is not None:
	result["http.target"] = target
	else:
	result["http.url"] = wsgiref_util.request_uri(environ)

I don't know under what circumstances the target is missing and the full URL is present instead, but I don't understand why only one or the other is included instead of both.

By comparison, the ASGI instrumentation happily includes both:

opentelemetry-python-contrib/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py

Lines 311 to 312 in 47caeab

	SpanAttributes.HTTP_TARGET: scope.get("path"),
	SpanAttributes.HTTP_URL: remove_url_credentials(http_url),

[qiuge615]

Hi @xrmx, regarding this issue, shall I add http_url for only new semconv as below PR's changes for asgi http_url?
#2735

Makes changes locally and the span is like below. Please review and if this makes sense I will create PR with changes, thanks.
('/hello', 'http://127.0.0.1:5000/hello')

[emdneto]

@qiuge615 In that case, the PR is removing the url.full attribute, which isn't required in server spans in new semantic conventions. Probably, this implementation is due to http.scheme, http.host and http.target being set, and it would be redundant to have http.url. These separate values are preferred, but if, for some reason, the URL is available, but the other values are not, the URL can replace the set of attributes: http.scheme, http.host, and http.target.

[qiuge615]

@emdneto Thanks for your help, url.full is not required in new semantic conventions. One question is regarding the old version of semantic conventions, http.url is required, as below doc, does that make sense to add it in old version of semantic conventions?
https://github.com/open-telemetry/opentelemetry-specification/blob/v1.20.0/specification/trace/semantic_conventions/http.md#http-client

[emdneto]

@qiuge615 http.url is required for http client instrumentations, which isn't the case here. Also, we are discouraging new changes that pertain to old semconv other than the transitions using the opt-in strategy. See, https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/CONTRIBUTING.md#guideline-for-instrumentations

[qiuge615]

@emdneto well received, thank your for your explanations.

[Kludex]

So... The ASGI adding the http.url is wrong?

[emdneto]

@Kludex the attribute isn't wrong. it's redundant for server spans in the new version of semconv (so, it is unnecessary to have it)