ref(remix): Isolate Express instrumentation from server auto-instrumentation. #9966
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Potentially fixes: #9963
We have been using a boolean flag
isRequestHandlerWrappedin the scope ofinstrumentServerto avoid instrumenting Remix build multiple times when an Express server adapter is manually instrumented too.The reason behind that decision was that we could not be sure that the
buildobject is the same, so looking for__sentry_original__on the wrapped function was not reliable all the time.While we still can't reproduce locally, #9963 showed that this strategy (hack) may not work all the time, so I tried to find an alternative reliable way to resolve this.
This PR introduces instrumentation for
getLoadContextwhich is available in all Remix server adapters, and passes data toloader/action/requestHandlers, to supply information about in which context a wrappedloader/action/requestHandleris used.We still run
instrumentBuildinsideSentry.initfor all Express / non-Express applications, but with this context, we can gauge the behaviour of instrumented functions, depending on whichinstrumentBuildthey're invoked from.So, with this PR we maintain two side-effect free flags:
__sentry_express_wrapped__from the instrumentedgetLoadContextmanuallyInstrumentedflag we pass toinstrumentBuildfunction which can be invoked either fromSentry.initorwrapExpressCreateRequestHandlerHaving those two, an instrumented
loader/action/requestHandlerwill behave accordingly to avoid creating multiple transactions.This approach can also be applied to injecting
baggageandsentry-traceas mentioned in #9737 in a new PR, if this gets merged.Additionally, this PR makes a slight change on the ordering of
hostlookup of RequestData, which I came across while testing this.Still not sure if this fixes the exact problem mentioned in #9963, but regardless IMHO it feels like a better resolution for this problem. cc: @souredoutlook