Fix for FASTAPI unable to record AppService URL (Issue #3654) #3670
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
| """Test that Host header values are properly decoded from bytes.""" | ||
| # This test verifies the fix for bytes vs string handling in our implementation | ||
| resp = self._client.get( | ||
| "/foobar", headers={"host": "bytes-test.example.com"} |
There was a problem hiding this comment.
| "/foobar", headers={"host": "bytes-test.example.com"} | |
| "/foobar", headers={"host": b"bytes-test.example.com"} |
am I missing something, or are we supposed to send the value as bytes here?
There was a problem hiding this comment.
No, it doesn't need to be bytes.
There was a problem hiding this comment.
I'm not even sure it accepts bytes? 👀
| # Find the server span (the main span, not internal middleware spans) | ||
| server_span = None | ||
| for span in spans: | ||
| if HTTP_URL in span.attributes: |
There was a problem hiding this comment.
I think will be better if we check if the span kind is the server? Same for other tests
| host_value = host_header[0] | ||
| # Ensure host_value is a string, not bytes | ||
| if isinstance(host_value, bytes): | ||
| host_value = host_value.decode("utf-8") |
There was a problem hiding this comment.
Can we use the already created _decode_header_item helper?
| # Current behavior: Host header may not affect server.address in new semantic conventions | ||
| # This test documents the current behavior rather than enforcing Host header usage | ||
| server_address = server_span.attributes.get("server.address", "") | ||
| self.assertIsNotNone(server_address) # Should have some value |
There was a problem hiding this comment.
Can we assert a more deterministic value?
|
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. |
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.