ref(node): Remove query strings from transaction and span names #2857
Conversation
size-limit report
|
There was a problem hiding this comment.
If we're removing the query string, we should also remove fragments.
The way this is implemented so far, it will remove query string and fragments when a query string exists (http://example.com/?q=1#top => http://example.com/), but will not remove fragments from URLs like http://example.com/#top.
|
If we're talking about grouping, we may also consider whether the let u = new URL(url);
return u.host + u.pathname;urlOrPath => {
let i = Math.min(urlOrPath.indexOf('#'), urlOrPath.indexOf('?'));
return i == -1 ? urlOrPath : urlOrPath.slice(0, i);
} |
|
We should also do this for Angular https://app.asana.com/0/1169125731075107/1191128420238986 Also @rhcarvalho I would go with just |
I think it's a different conversation depending on whether we're talking about incoming or outgoing requests. Incoming requests (for which we create transactions) will presumably always be to Outgoing requests (for which we create spans) are highly likely to be to outside domains, so we have historically taken (and I think we should continue to take) the full URL. * @HazAT, you mentioned parameterizing the path ( |
Good point. Thanks for catching that!
Wouldn't this no-op on any url that didn't have both a Anyway, this seems to work: if (urlPath.includes('?') || urlPath.includes('#')) {
// in theory, the fragment should always come after the query string, but you never know, so make
// the # a ? so the split will always happen if either is there
return urlPath.replace('#', '?').split('?')[0];
} |
| // internal routes end up with too many slashes | ||
| return `${protocol}//${hostname}${port}${path}`.replace('///', '/'); |
There was a problem hiding this comment.
This is a very poor-man's slash normalization.
We'd get this for free if using new Url(...).
There was a problem hiding this comment.
Can you say more here? I couldn't find any reference to a Url class in the MDN docs, and the URL class doesn't seem to be able to deal with just a path (even if it starts with three slashes):
There was a problem hiding this comment.
When do we even have the case when we have too many slashes? Afaik ClientRequest is printing http or https, without any slashes
There was a problem hiding this comment.
See my reply below: #2857 (comment)
packages/node/src/handlers.ts
Outdated
| @@ -32,7 +32,7 @@ export function tracingHandler(): ( | |||
| // TODO: At this point `req.route.path` (which we use in `extractTransaction`) is not available | |||
| // but `req.path` or `req.url` should do the job as well. We could unify this here. | |||
| const reqMethod = (req.method || '').toUpperCase(); | |||
| const reqUrl = req.url; | |||
| const reqUrl = req.url && stripUrlPath(req.url); | |||
There was a problem hiding this comment.
How about dropping req.url && here and making stripUrlPath return the input directly if it's not a string?
There was a problem hiding this comment.
That would mean adding undefined to both the input and output types, which then would just push the &&ing to the other end (because it the result wouldn't be guaranteed to be defined) - no? Is that better? (Happy to do it if you think so.)
| // internal routes end up with too many slashes | ||
| return `${protocol}//${hostname}${port}${path}`.replace('///', '/'); |
There was a problem hiding this comment.
When do we even have the case when we have too many slashes? Afaik ClientRequest is printing http or https, without any slashes
| describe('tracingHandler', () => { | ||
| const urlString = 'http://dogs.are.great:1231/yay/'; | ||
| const queryString = '?furry=yes&funny=very'; | ||
| const fragment = '#adoptnotbuy'; |
There was a problem hiding this comment.
When do we even have the case when we have too many slashes? Afaik ClientRequest is printing http or https, without any slashes
If it's an internal route, there's no protocol or hostname, and it looks like this:
Including query strings in the URLs we use to name transactions and spans has at least two drawbacks:
This removes the query string from the URLs we use to name both transactions representing incoming requests and spans representing outgoing requests. It also fixes a special case in which some outgoing requests to external servers were missing the protocol, and adds tests not only for this behavior but for the tracing handler in general.
Before:
After: