Skip to content
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.

Sign up for GitHub

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

Jump to bottom

RequestData integration prework, work, and postwork #5756

Closed
10 of 20 tasks
lobsterkatie opened this issue Sep 16, 2022 · 3 comments
Closed
10 of 20 tasks

RequestData integration prework, work, and postwork #5756

lobsterkatie opened this issue Sep 16, 2022 · 3 comments
Assignees
@lobsterkatie

Comments

@lobsterkatie
Copy link
Member

lobsterkatie commented Sep 16, 2022 •
edited

There have gotten to be enough moving parts and dependencies involved in the RequestData integration work that I figured it's worth writing them all down.

Background

  • The underlying request-data-adding functions got moved from @sentry/node to @sentry/utils in order to be able to use them in the _error helper in the nextjs SDK, which runs in both browser and node and therefore must be platform agnostic. (The function is only actually called in node, but has to be imported from a neutral source.) As a result of that change, node-specific dependencies needed for those functions were forced to be provided via dependency injection, by wrapper functions in the node SDK.
  • The new data-fetcher wrappers in the nextjs SDK needed a new way to use the functions. (We currently use them in scope-specific event processors, but the events those wrappers create pass through a number of different scopes, so that won't work for them.) To solve this, and to bring continuity to our use of the functions, we decided to create an integration.
  • There was a recent DACI wherein we decided that we eventually want to add the same data to events tracking fetch requests in the browser.

Problems/Constraints

  • The functions themselves are part of the Great Transaction Mess (see Clean up code relating to scope/event transaction value  #5718 and Problematic API: Scope.setTransactionName() #5660).
  • The dependency injection is kind of gross and hard to follow.
  • It's unclear how much overlap there'll be between the node and browser versions of the integration, and the browser version isn't yet roadmapped (and therefore we don't know how soon it will happen).
  • Because of the above two points, we decided to undo the move from node to utils, because once the integration is in use, the _error helper doesn't need to use the functions directly. Until it is, though, the helper still needs the function to live in the utils package.

Order of Operations

Immediate term:

Longer term:

  • Clean up transaction mess
  • Move extra logic from integration to revamped request-data-adding functions
  • Once browser version of integration is needed, figure out if we want to factor out common functionality or just make a separate integration

v8 Removals/Deprecations/Breaking Changes:

(WIP - may not be an exhaustive list)

  • Remove requestDataDeprecated.ts (parseRequest and friends)
  • Remove tests testing parseRequest/addRequestDataToEvent compatibility
  • Remove utils copy of request data functions
  • Make express request handler no longer take request data options (users can use integration options)
  • Make CGP wrapper no longer take request data options (users can use integration options)
  • Remove helper functions converting between parseRequest, addRequestDataToEvent, and RequestData integration options
@lobsterkatie lobsterkatie mentioned this issue Sep 16, 2022
Merged
lobsterkatie added a commit that referenced this issue Sep 17, 2022
@lobsterkatie
ref(node): Move request data functions back to@sentry/node (#5759)
9862a32 
As part of the work adding a `RequestData` integration, this moves the `requestdata` functions 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 `_error` helper 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.
@lobsterkatie lobsterkatie mentioned this issue Sep 27, 2022
Closed
43 tasks
@vladanpaunovic
Copy link
Contributor

@lobsterkatie , looking at this ticket, can you please clarify what is the impact on the user here?

@lobsterkatie
Copy link
Member Author

@lobsterkatie , looking at this ticket, can you please clarify what is the impact on the user here?

This is partially background work (standardizing our approach generally makes maintenance and expansion to other frameworks easier) and partially in order to bring our express integration's ability to, for example, not send cookies data to all packages where we attach request data to an event. (See this issue, for example: #4723.)

This was referenced Oct 19, 2022
Merged
Merged
lobsterkatie added a commit that referenced this issue Oct 20, 2022
@lobsterkatie
ref(node): Use RequestData integration in express handlers (#5990)
1f99c3b 
This switches the request, tracing, and error handlers from the Node SDK's `handlers.ts` to use the new `RequestData` integration for adding request data to events rather than the current event processor. 

Notes:

- So that this isn't a breaking change, for the moment (read: until v8) the integration will use the options passed to the request handler in place of its own options. (The request handler now stores its options in `sdkProcessingMetadata` alongside the request so that the integration will have access to them.)
- Before this change, the event processor was backwards-compatible by dint of calling `parseRequest` rather than `addRequestDataToEvent` if the request handler's options are given in the old format. Because the integration uses only `addRequestDataToEvent` under the hood, the backwards compatibility is now achieved by converting the old-style options into equivalent new-style options, using a new helper function `convertReqHandlerOptsToAddReqDataOpts`. (And yes, that function name is definitely a mouthful, but fortunately only one will has to last until v8.)
- Though in theory one should never use the error or tracing middleware without also using the request middleware, people do all sorts of weird things. All three middlewares therefore add the request to `sdkProcessingMetadata`, even though just doing so in the request handler should technically be sufficient.

Ref: #5756
lobsterkatie added a commit that referenced this issue Oct 20, 2022
@lobsterkatie
ref(serverless): Use RequestData integration in GCP wrapper (#5991)
0576852 
This switches the GCP wrapper in the serverless SDK to use the new `RequestData` integration for adding request data to events rather than the current event processor. 

Ref: #5756
@HazAT
Copy link
Member

HazAT commented Feb 16, 2023

Closing for now - we might reconsider this in the future

@HazAT HazAT closed this as completed Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lobsterkatie lobsterkatie

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@HazAT @lobsterkatie @vladanpaunovic