feat(stdlib): Handle proxy tunnels in httlib integration #5303
Conversation
sl0thentr0py
force-pushed
the
neel/stdlib-proxy
branch
from
7c49a36 to
bf37a48
Compare
sl0thentr0py
changed the title
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Documentation 📚
Internal Changes 🔧
🤖 This preview updates automatically when you update the PR. |
sentry_sdk/integrations/stdlib.py
Outdated
|
|
||
| # Set network peer address and port (the actual connection endpoint) | ||
| # for proxies, these point to the proxy host/port | ||
| span.set_data(SPANDATA.NETWORK_PEER_ADDRESS, self.host) |
There was a problem hiding this comment.
these are according to otel spec
![sentry[bot]](/s/com/githubusercontent/avatars/G.https/u/1396951?s=60&v=4){kind=small}
| tunnel_port = getattr(self, "_tunnel_port", None) | ||
| if tunnel_port: | ||
| port = tunnel_port | ||
| # need to override default_port for correct url recording | ||
| if tunnel_port == 443: | ||
| default_port = 443 | ||
| else: | ||
| port = self.port |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![cursor[bot]](/s/com/githubusercontent/avatars/G.https/in/1210556?s=60&v=4){kind=small}
| if tunnel_port == 443: | ||
| default_port = 443 | ||
| else: | ||
| port = self.port |
There was a problem hiding this comment.
Proxy port used when tunnel port is unspecified
Low Severity
When _tunnel_host is set but _tunnel_port is None (because set_tunnel was called without specifying a port), the code falls back to self.port which is the proxy port, not the tunnel destination port. This causes incorrect URL recording in the span. For example, if using HTTPConnection("proxy", 8080).set_tunnel("api.example.com") (no port), the recorded URL would incorrectly include :8080. The else branch assumes no tunnel is in use, but _tunnel_host being set indicates tunnel mode where the default port (80/443) is more appropriate than the proxy port.
| tunnel_port = getattr(self, "_tunnel_port", None) | ||
| if tunnel_port: | ||
| port = tunnel_port | ||
| # need to override default_port for correct url recording |
There was a problem hiding this comment.
I'd suggest changing the url recording logic instead.
Otherwise, looks good to me!
Issues