feat(nextjs): Parameterize server-side transaction names and harvest request data #3577
Conversation
size-limit report
|
|
|
||
| // requests for pages will only ever be GET requests, so don't bother to include the method in the transaction | ||
| // name; requests to API routes could be GET, POST, PUT, etc, so do include it there | ||
| const namePrefix = req.url.startsWith('/api') ? `${(req.method || 'GET').toUpperCase()} ` : ''; |
There was a problem hiding this comment.
@HazAT pinging Daniel to give his thoughts on the name prefix for transaction
There was a problem hiding this comment.
This is what we agreed to in our meeting, no? API requests might have different methods, but page requests are always GET, so include the method for API calls, and not for the page ones. I believe I even confirmed it with him afterwards.
| const transaction = Sentry.startTransaction({ | ||
| name: `${namePrefix}${reqPath}`, | ||
| op: 'http.server', | ||
| metadata: { request: req }, |
There was a problem hiding this comment.
Can we store the req.method as a tag?
There was a problem hiding this comment.
Sure, we can. I'm curious why, though, given that it's in the transaction name if it's anything other than GET.
| if (transaction && transaction.metadata.request) { | ||
| // strip query string, if any | ||
| const origPath = transaction.metadata.request.url.split('?')[0]; | ||
| transaction.name = transaction.name.replace(origPath, parameterizedPath); |
There was a problem hiding this comment.
Why do we do a .replace here instead of just overwriting the URL?
There was a problem hiding this comment.
Also will this affect users manual transactions as well?
There was a problem hiding this comment.
I used replace because some of the transaction names have the method, and I only want to overwrite the URL part. As for manual transactions, it would only affect them if they a) store the correct metadata on the transaction, and b) have a request URL as part or all of their transaction name. Though not technically impossible... it still feels unlikely.
lobsterkatie
force-pushed
the
kmclb-nextjs-parameterize-transaction-name
branch
from
40bab14 to
f960139
Compare
This PR does two things:
It wraps two more methods on
next'sServerclass, one of which is called on every API request, and the other of which is called on every page request. Both methods are passed a parameterized version of the path, which our wrappers grab and use to modify the name of the currently active transaction.It adds an event processor which pulls data off of the request and adds it to the event. This runs whether or not tracing is enabled, in order to add data to both error and transaction events.
The only other thing of minor interest is that the type for
Event.request.query_stringis expanded to include objects, which is okay according to the dev docs.