ref(nextjs): Clarify user integration functions #3599
Closed
Conversation
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
Adds front-end performance monitoring in the Next.js SDK, by providing a router instrumentation.
…request data (#3577) This PR does two things: 1) It wraps two more methods on `next`'s `Server` class, one of which is called on every API request, and the other of which is called on every page request. Both methods are passed a parameterized version of the path, which our wrappers grab and use to modify the name of the currently active transaction. 2) It adds an event processor which pulls data off of the request and adds it to the event. This runs whether or not tracing is enabled, in order to add data to both error and transaction events. The only other thing of minor interest is that the type for `Event.request.query_string` is expanded to include objects, which is okay according to the dev docs: https://develop.sentry.dev/sdk/event-payloads/request/#attribute).
Just a bunch of random housekeeping. - No longer return project root directory from `instrumentServer` since it's not yet defined at that point. - No longer store transaction on `res`, since our new use of domains means that `getActiveTransaction` is safe. As a result, change `NextResponse` from an interface to a type which is purely an alias. - Store a pointer to the live `Server` instance directly on the top scope rather than in an object, and get rid of the other stuff being stored in that object (`closure`) since it can all be gotten off of the aforementioned `Server` instance. - Fix/expand/tweak various docstrings and comments. - Improve log message when tracing an outgoing HTTP request.
1) Fix a bug which prevented the "don't overwrite these keys" warning from working correctly. 2) Restrict the warning to appearing when overwriting keys which actually have values.
Automatically enable `BrowserTracing` if a traceSampler or tracesSample Rate is set. Co-authored-by: Abhijeet Prasad <aprasad@sentry.io>
Contributor
size-limit report
|
kamilogorek
previously requested changes
May 26, 2021
AbhiPrasad
reviewed
May 26, 2021
| BrowserTracing: { keyPath: 'options.routingInstrumentation', value: nextRouterInstrumentation }, | ||
| }); | ||
| return addIntegration(defaultBrowserTracingIntegration, integrations, [ | ||
| ['options.routingInstrumentation', nextRouterInstrumentation], |
Member
There was a problem hiding this comment.
Why not pass in the integration name here? I think that would simplify the logic when trying the find the integration to update.
Member
Author
There was a problem hiding this comment.
Yeah, I thought of that, but I was trying not to completely redo everything, LOL. Happy to do it, though.
Damn, didn't read closely enough. You mean instead of passing in a fully-formed instance? I'm not sure how that makes things simpler.
Member
There was a problem hiding this comment.
Something like this (like the logic I had above), so we know exactly what integration to update.
Suggested change
| ['options.routingInstrumentation', nextRouterInstrumentation], | |
| ['BrowserTracing', 'options.routingInstrumentation', nextRouterInstrumentation], |
then we can just
for (const integration of allIntegrations) {
if integration.name === options[0] {
update(integration, options[1], options[2]);
}
}
Member
Author
|
Closing this as stale, as I think some of these changes have been made in the meantime. I'll come back to the others at some point, but I think it'll be easiest to do that in a new PR. |
lobsterkatie
deleted the
kmclb-clarify-user-integration-manipulation
branch
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.
With @AbhiPrasad's blessing, a bunch of edits which originated as suggestions to #3586. That got merged before they could be made, so this adds them in a different form.
Included changes:
addIntegrationToArray, to make the conditionals and flow clearerOptionstype, to enforce that only options for the given integration can be passed and to allow multiple options to be passed