feat(core): Add server.address to browser http.client spans
#11634
Conversation
size-limit report 📦
|
Good point, was surprised we have nothing for this actually, I added tests to ensure we check the span data for the important stuff at least :) |
mydea
force-pushed
the
fn/add-server-request
branch
from
5642fad to
c4a5142
Compare
|
OK, after @gggritso's comment I actually added tests for relative URLs, which showed the initial implementation was flawed! Now it should work - it's a bit more complicated because for the base fetch instrumentation, we can't access |
| const host = parseUrl(fullUrl).host; | ||
| createdSpan.setAttributes({ | ||
| 'http.url': fullUrl, | ||
| 'server.address': host, |
There was a problem hiding this comment.
A small ask, but could you add server.port, too while you're here? It'd made life a lot easier in Relay when creating span metrics tags!
There was a problem hiding this comment.
sadly it's not so easy to get the port, as most URLs don't have it and we just don't really know. We could "guess" it based on the protocol but this seems tricky to me? Anyhow, we can track this in a separate issue if we need this, I'd say!
| @@ -76,6 +76,8 @@ test('Should trace outgoing fetch requests inside middleware and create breadcru | |||
| 'http.response.status_code': 200, | |||
| type: 'fetch', | |||
| url: 'http://localhost:3030/', | |||
| 'http.url': 'http://localhost:3030/', | |||
| 'server.address': 'localhost:3030', | |||
There was a problem hiding this comment.
@gggritso regarding port, you can see here that the port is part of the server.address if it is in the URL (=non-standard)
Closes #11632