Skip to content
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

opentelemetry-instrumentation-asgi: do not set url.full attribute for server spans #2698

Closed
emdneto opened this issue Jul 11, 2024 · 7 comments · Fixed by #2735
Closed

opentelemetry-instrumentation-asgi: do not set url.full attribute for server spans #2698

emdneto opened this issue Jul 11, 2024 · 7 comments · Fixed by #2735
Assignees
Labels
bug Something isn't working

Comments

@emdneto
Copy link
Member

emdneto commented Jul 11, 2024

Describe your environment

instrumentation-asgi: 0.47b0.dev

What happened?

asgi instrumentation is emitting url.full attribute in server spans

Steps to Reproduce

Run any example of asgi instrumentation with new semconv opt-in enabled

import fastapi
import os
import logging
from opentelemetry.instrumentation.fastapi import FastAPIInstrumentor
from opentelemetry import trace
from opentelemetry.sdk.trace import TracerProvider, sampling
from opentelemetry.sdk.resources import Resource
from opentelemetry.sdk.trace.export import (
    BatchSpanProcessor,
    ConsoleSpanExporter,
)
# Specify "http", "http/dup" or ""
os.environ["OTEL_SEMCONV_STABILITY_OPT_IN"] = "http"
logging.basicConfig(level=logging.DEBUG)

provider = TracerProvider()
processor = BatchSpanProcessor(ConsoleSpanExporter())
provider.add_span_processor(processor)
trace.set_tracer_provider(provider)
tracer = trace.get_tracer(__name__)

app = fastapi.FastAPI()

@app.get("/foobar")
async def foobar():
    return {"message": "hello world"}

FastAPIInstrumentor.instrument_app(app)

Expected Result

"attributes": {
        "url.scheme": "http",
        "server.address": "127.0.0.1:8000",
        "server.port": 8000,
        "network.protocol.version": "1.1",
        "url.path": "/foobar",
        "http.request.method": "GET",
        "user_agent.original": "curl/7.68.0",
        "client.address": "127.0.0.1",
        "client.port": 45854,
        "http.route": "/foobar",
        "http.response.status_code": 200
    },

Actual Result

Server span Attributes with url.full which isn't necessary for server span:

"attributes": {
        "url.scheme": "http",
        "server.address": "127.0.0.1:8000",
        "server.port": 8000,
        "network.protocol.version": "1.1",
        "url.path": "/foobar",
        "url.full": "http://127.0.0.1:8000/foobar",
        "http.request.method": "GET",
        "user_agent.original": "curl/7.68.0",
        "client.address": "127.0.0.1",
        "client.port": 45854,
        "http.route": "/foobar",
        "http.response.status_code": 200
    },

Additional context

Seems we are setting http.url (for old semconv) since always but isn't really necessary anymore

#2682 (comment)

https://github.com/open-telemetry/opentelemetry-specification/blob/v1.11.0/specification/trace/semantic_conventions/http.md#http-server-semantic-conventions

Would you like to implement a fix?

None

@emdneto emdneto added the bug Something isn't working label Jul 11, 2024
@emdneto emdneto changed the title opentelemetry-instrumentation-asgi: do not set url.full attribute for server spans and metrics opentelemetry-instrumentation-asgi: do not set url.full attribute for server spans Jul 11, 2024
@shijiadong2022
Copy link
Contributor

I'd like to work on this one and will submit a PR

@shijiadong2022
Copy link
Contributor

shijiadong2022 commented Jul 12, 2024

@emdneto Is the expectation to remove url.full attribute for both old and new semconv?

@lzchen
Copy link
Contributor

lzchen commented Jul 12, 2024

@shijiadong2022

Seems like we've always recorded http.url for old sem conv so this change would just be removing it for url.full for new semconv.

@emdneto
Copy link
Member Author

emdneto commented Jul 12, 2024

Right. I would just pay attention to http/dup mode, according to the migration guide:

http/dup - emit both the old and the stable HTTP and networking conventions, allowing for a phased rollout of the stable semantic conventions.

@emdneto
Copy link
Member Author

emdneto commented Jul 17, 2024

@shijiadong2022 are you stil working on this?

@shijiadong2022
Copy link
Contributor

I am working on it.

@shijiadong2022
Copy link
Contributor

@emdneto when you get a chance can you review my PR? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants