ref(node): Move request data functions back to@sentry/node
#5759
Conversation
This file contains 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
20 tasks
size-limit report 📦
|
AbhiPrasad
approved these changes
Sep 16, 2022
lobsterkatie
force-pushed
the
kmclb-move-requestdata-functions-back-to-node-sdk
branch
from
55c94dc
to
a175108
Compare
lobsterkatie
force-pushed
the
kmclb-move-requestdata-functions-back-to-node-sdk
branch
from
a175108
to
05709dc
Compare
lobsterkatie
deleted the
kmclb-move-requestdata-functions-back-to-node-sdk
branch
4 tasks
lobsterkatie
added a commit
that referenced
this pull request
Sep 19, 2022
#5703) In most of our Node-based SDKs, we use domains to prevent scope bleed between requests, running the entire wrapped event-handling process inside a single domain. This works because in those cases, there is only one request-handling entry point for us to wrap, so we can stick the entire thing in a single `domain.run()` call and (more or less) rely on the fact that we've got a single, consistent, unique-to-that-request `Scope` object that we're dealing with. We've followed this pattern in the nextjs SDK as well, both in `instrumentServer` and `withSentry`. The new auto-wrapping of data-fetching functions presents a challenge, though, because a single request may involve a number of our wrapped functions running at different points in the request handling process, with no shared (little S) scope between them. While we still use domains when wrapping the data fetchers, a single request may pass through multiple domains during its lifecycle, preventing us from using any given domain as a universal `Scope`-carrier for a single `Scope` instance. One place where this becomes is problem is our use of `addRequestDataToEvent`, which up until now we've just been calling inside a single-request event processor added to the current `Scope`. In this system, each event processor holds a reference to its particular `req` object. In the case of the data-fetchers, however, the `Scope` instance to which we might add such an event processor isn't the same as the one which will be active when the event is being processed, so the current way doesn't work. But given that the only way in which our current, single-request-specific event processors differ is by the request to which they hold a reference, they can be replaced by a single, and universal, event processor, as long as we can access `req` a different way besides keeping it in a closure as we do now. This PR does just that (that is, switches to using a single event processor) for transaction events. First, a reference to `req` is stored in the transaction's metadata (which is then available to event processors as `sdkProcessingMetadata`). Then a new default integration, `RequestData`, pulls `req` out of the metadata and uses it to add a `request` property to the event. Notes: - The options API for the new integration is inspired by, but different from, the options API for our Express request handler. (When we work on cleaning up the request data utility functions as part of fixing #5718, we might consider bringing those options in line with these.) The primary differences are: - The options have been (almost entirely) flattened. (It never made sense that inside of options for request data, you had a `request` key holding a subset of the options.) Now everything is at the same level and only takes a boolean. The one exception is `user`, which can still take a boolean or a list of attributes. - In the handler options, `transaction` can either be a boolean or a `TransactionNamingScheme`. In the integration, it can no longer be a boolean - events are going to have transactions, one way or another, so we shouldn't let people set it to `false`. Further, since it's now not about whether `transaction` is or isn't included, it's been moved out of `include` into its own `transactionNamingScheme` option. (Note that the new option is not yet used, but will be in future PRs.) - `method` has also been moved out of include, and has been hardcoded to `true`, since it's an integral part of naming the request. We currently include it in the transaction name, regardless of the setting, so again here, letting people set it to `false` makes no sense. - Though `req` has been added to transaction metadata everywhere we start transactions, the existing domain/`Scope`-based event processors haven't yet been removed, because this new method only works for transactions, not errors. (Solving that will be the subject of a future PR.) The existing processors have been modified to not apply to transaction events, however. - Though we may at some point use the `RequestData` integration added here in browser-based SDKs as well, both in this PR and in near-term future PRs it will only be used in a Node context. It's therefore been added to `@sentry/node`, to prevent a repeat of the dependency injection mess [we just undid](#5759). - No integration tests were added specifically for this change, because the existing integration tests already test that transaction events include a `request` property, so as long as they continue to pass, we know that using the integration instead of an event processor is working. Ref: #5505
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.
As part of the work adding a
RequestDataintegration, this moves therequestdatafunctions back into the node SDK. (The dependency injection makes things hard to follow, and soon the original reason for the move (so that they could be used in the_errorhelper in the nextjs SDK, which runs in both browser and node) will no longer apply (once #5729 is merged).)Once these functions are no longer needed, they can be deleted from
@sentry/utils.More details and a work plan are in #5756.