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:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/47caeab7aff47070c565cd90536a39ec5eb06f34/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py#L320-L325

The first commit of the file shows essentially the same:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/d19b464a0a23a4a66d5df635634e7d6315cb54db/instrumentation/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py#L108-L111

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:

https://github.com/open-telemetry/opentelemetry-python-contrib/blob/47caeab7aff47070c565cd90536a39ec5eb06f34/instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py#L311-L312

[qiuge615]

Hi @xrmx, regarding this issue, shall I add http_url for only new semconv as below PR's changes for asgi http_url? https://github.com/open-telemetry/opentelemetry-python-contrib/pull/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)