feat(core)!: Update requestDataIntegration handling
#14806
Merged
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
size-limit report 📦
|
This was referenced Dec 30, 2024
mydea
added a commit
that referenced
this pull request
Dec 30, 2024
Fixes #14847 After some digging, I figured out that the `req.user` handling on express is not working anymore, because apparently express does not mutate the actual http `request` object, but clones/forkes it (??) somehow. So since we now set the request in the SentryHttpInstrumentation generally, it would not pick up express-specific things anymore. IMHO this is not great and we do not want this anymore in v9 anyhow, but it was the behavior before. This PR fixes this by setting the express request again on the isolation scope in an express middleware, which is registered by `Sentry.setupExpressErrorHandler(app)`. Note that we plan to change this behavior here: #14806 but I figured it still makes sense to fix this on develop first, so we have a proper history/tests for this. I will backport this to v8 too. Then, in PR #14806 I will remove the middleware again.
mydea
added a commit
that referenced
this pull request
Jan 3, 2025
Instead of picking this from the plain `request` in `requestDartaIntegration`. Extracted from #14806
mydea
added a commit
that referenced
this pull request
Jan 7, 2025
Instead of picking this from the plain `request` in `requestDartaIntegration`. Extracted from #14806
mydea
added a commit
that referenced
this pull request
Jan 7, 2025
This was an express-specific, rather undocumented behavior, and also conflicted with our privacy-by-default stance. Starting in v9, you'll need to manually call `Sentry.setUser()` e.g. in a middleware to set the user on Sentry events. Docs for this are already pending: getsentry/sentry-docs#12224 Extracted this out of #14806
mydea
force-pushed
the
fn/normalized-request
branch
from
4f9f9f7 to
72f6880
Compare
mydea
changed the title
requestDataIntegration handlingrequestDataIntegration handling
mydea
commented
Jan 8, 2025
| } | ||
| }); | ||
| } catch { | ||
| // just return the empty headers |
There was a problem hiding this comment.
we used to log a debug message here, no we just continue.
mydea
commented
Jan 8, 2025
| } | ||
| }); | ||
| } catch { | ||
| // just return the empty headers |
There was a problem hiding this comment.
we used to log a debug message here, no we just continue.
mydea
commented
Jan 8, 2025
| const protocol = request.protocol || (request.socket?.encrypted ? 'https' : 'http'); | ||
| const url = request.url || ''; | ||
|
|
||
| const absoluteUrl = getAbsoluteUrl({ |
There was a problem hiding this comment.
previously, we always built this from the host. However, the host could also be <no host> as fallback, leading to (theoretically) urls like: http://<no host>/ which is weird. Now, we simply have no URL in such a case.
mydea
commented
Jan 8, 2025
packages/core/src/utils/request.ts
Outdated
| protocol, | ||
| host, | ||
| }: { url?: string; protocol: string; host?: string }): string | undefined { | ||
| if (url && url.startsWith('http')) { |
There was a problem hiding this comment.
previously, we checked here url.startsWith(protocol). Which should be generally fine, but there could be an edge case where you have url: http://example.com and protocol: https which would result in https://http://example.com which seems bad.
mydea
force-pushed
the
fn/normalized-request
branch
from
72f6880 to
fd5d6ab
Compare
lforst
approved these changes
Jan 10, 2025
| @@ -60,7 +59,6 @@ export interface SdkProcessingMetadata { | |||
| requestSession?: { | |||
| status: 'ok' | 'errored' | 'crashed'; | |||
| }; | |||
| request?: PolymorphicRequest; | |||
mydea
force-pushed
the
fn/normalized-request
branch
from
fd5d6ab to
3d17c65
Compare
s1gr1d
approved these changes
Jan 10, 2025
mydea
force-pushed
the
fn/normalized-request
branch
from
3d17c65 to
95fb966
Compare
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.
This PR cleans up the behavior of
requestDataIntegration. For this, there are a bunch of parts that come together:addNormalizedRequestDataToEventexport, this does not really do much, you can just directly add the data to the event.RequestDataIntegrationOptionstyperequeston SDK processing metadata, now only relying onnormalizedRequestutils-hoist. Now, all the code related directly to the requestDataIntegration is in that file, while the more general utils are in core/src/utils/request.tsCloses #14297