-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(remix): Continue transaction from request headers #5600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Looks like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make sure there is some test that validates this?
Yup - first have to figure out why existing tests are failing, then we add a new test. |
5b25a5e
to
299e93b
Compare
size-limit report 📦
|
async function makeRequest( | ||
method: 'get' | 'post' = 'get', | ||
url: string, | ||
axiosConfig?: AxiosRequestConfig, | ||
): Promise<void> { | ||
try { | ||
if (method === 'get') { | ||
await axios.get(url); | ||
await axios.get(url, axiosConfig); | ||
} else { | ||
await axios.post(url); | ||
await axios.post(url, axiosConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts on this approach for adding axois config @onurtemizkan? Should we do it some other way? Maybe by putting base axios config in the TestEnv
constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this approach (passing to the data method) with a default header set in TestEnv
would make it easier if we want to test consecutive requests with different configurations?
I have just tested setting the default inside class, and it's not breaking any of the current Node / Remix tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extract this to be a setter on the class, lmk what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
This is now ready to review! |
const matches = matchServerRoutes(routes, url.pathname, pkg); | ||
const match = matches && getRequestMatch(url, matches); | ||
const name = match === null ? url.pathname : match.route.id; | ||
const source = match === null ? 'url' : 'route'; | ||
const [name, source]: [string, TransactionSource] = | ||
match === null ? [url.pathname, 'url'] : [match.route.id, 'route']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a suggestion, don't feel obligated to act: I believe we could do the matching of the routes to the request url outside of this function. That way we don't have to pass in url
, routes
and pkg
but could instead just pass a transactionName
argument.
That would probably mean though that we duplicate the route matching logic in instrumentServer.ts
and the express.ts
adapter. Maybe we could extract this whole route matching + source determining into a separate function? Anyways, just a suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion, done with fc4f653
const transaction = startRequestHandlerTransaction(hub, url, routes, pkg, { | ||
headers: { | ||
'sentry-trace': (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '', | ||
baggage: (req.headers && isString(req.headers.baggage) && req.headers.baggage) || '', | ||
}, | ||
method: request.method, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enables propogation of traces through
sentry-trace
and dynamic sampling propogation throughbaggage