Fix for FASTAPI unable to record AppService URL (Issue #3654)#3670
Fix for FASTAPI unable to record AppService URL (Issue #3654)#3670xrmx merged 30 commits intoopen-telemetry:mainfrom
Conversation
|
Before Change - 
After Change - 
|
There was a problem hiding this comment.
Left some comments. One thought though is we don't even need the HTTP_URL attribute for server spans even for old semantic convention, but I believe it's another discussion.
http.url is usually not readily available on the server side but would have to be assembled in a cumbersome and sometimes lossy process from other information
|
I think the current implementation is correct. I don't think the instrumentation should trust the |
But then the IP address appearing in the url field instead of the actual application url address is not very desirable. @Kludex Do you think there is another way to overcome this issue. |
|
Long needed fix. Thanks @rads-1996! Fastapi and asgi should behave like the other instrumentations and use the url when available. |
rads-1996
force-pushed
the
fix-fastapi-url
branch
from
ca9a110 to
922335c
Compare
There was a problem hiding this comment.
I agree host header is 100% safe, but approving because it looks good to me and is consistent with the WSGI instrumentation. Also, is the way semantic conventions recommends to get the hostname https://opentelemetry.io/docs/specs/semconv/http/http-spans/#setting-serveraddress-and-serverport-attributes. btw, for new semconv we are not recording it #2698
…emetry/instrumentation/asgi/__init__.py
…#3654) (open-telemetry#3670) * Fix for FASTAPI unable to record AppService URL * Fixed tests and pylint errors * Fixed ruff format * Updated CHANGELOG * Updated CHANGELOG * Addressed feedback * Checking CI runs * Fix ruff and spellcheck errors * Fix pytest * Update CHANGELOG.md * Update CHANGELOG.md * Update instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py * Fix ruff * Fix ruff * Updated CHANGELOG --------- Co-authored-by: Emídio Neto <[email protected]> Co-authored-by: Riccardo Magliocchetti <[email protected]>
5 participants
Description
Fixes #3654
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.