ref(remix): Isolate Express instrumentation from server auto-instrumentation.#9966
Merged
ref(remix): Isolate Express instrumentation from server auto-instrumentation.#9966
Conversation
Contributor
size-limit report 📦
|
30c78ea to
bb04a33
Compare
isRequestHandlerWrapped flag into scope.bb04a33 to
e8a1841
Compare
lforst
approved these changes
Dec 29, 2023
Contributor
lforst
left a comment
There was a problem hiding this comment.
lgtm! If, for some reason, this doesn't end up working or isn't robust enough we could investigate marking individual request objects as instrumented.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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