fix(nextjs): Remove Http integration from Next.js #11304
Merged
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
s1gr1d
force-pushed
the
sig-nextjs-remove-http
branch
from
c10f4ed to
2184334
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
lforst
reviewed
Mar 27, 2024
|
|
||
| return new Proxy(routeHandler, { | ||
| apply: (originalFunction, thisArg, args) => { | ||
| return withIsolationScope(async isolationScope => { |
There was a problem hiding this comment.
h: Since the http integration was responsible for creating the isolation scope, we need to add the call to withIsolationScope back, otherwise we'll mutate the global isolation scope and leak request data into other requests.
s1gr1d
force-pushed
the
sig-nextjs-remove-http
branch
3 times, most recently
from
a9e26e9 to
b3bc50f
Compare
size-limit report 📦
|
s1gr1d
force-pushed
the
sig-nextjs-remove-http
branch
3 times, most recently
from
d41b917 to
37649af
Compare
13 tasks
mydea
reviewed
Apr 5, 2024
| const spanJson = spanToJSON(span); | ||
|
|
||
| // The following check is a heuristic to determine whether the started span is a span that tracks an incoming HTTP request | ||
| if (getSpanKind(span) === SpanKind.SERVER && spanJson.data && 'http.method' in spanJson.data) { |
There was a problem hiding this comment.
m: Let's use otel semantic attributes here (from @opentelemetry/semantic-conventions), I'd say?
There was a problem hiding this comment.
Yeah, that makes sense! Here: #11467
This comment was supposed to be elsewhere, I added it to the correct place
mydea
reviewed
Apr 5, 2024
mydea
reviewed
Apr 5, 2024
| [SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE]: sampleRate, | ||
| }; | ||
|
|
||
| const method = `${spanAttributes[SemanticAttributes.HTTP_METHOD]}`.toUpperCase(); |
There was a problem hiding this comment.
Let's pull this into a separate PR!
There was a problem hiding this comment.
Yeah, that makes sense! Here: #11467
mydea
force-pushed
the
sig-nextjs-remove-http
branch
from
37649af to
786b4cb
Compare
s1gr1d
force-pushed
the
sig-nextjs-remove-http
branch
from
364119c to
6f17db7
Compare
mydea
approved these changes
Apr 9, 2024
| rootSpan.updateName(spanName); | ||
| rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); | ||
| rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'http.server'); | ||
| rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, 'auto.function.nextjs'); |
There was a problem hiding this comment.
l: In a follow up, we can extend this to also set method and other stuff we possibly get there. But no need to do that now!
Side note, you can also use rootSpan.setAttributes({ ... }) to set multiple attributes at once :)
This reverts commit fda5f32.
Just trying some things...
Co-authored-by: Francesco Novy <[email protected]>
s1gr1d
force-pushed
the
sig-nextjs-remove-http
branch
from
6bc00da to
1674e11
Compare
cadesalaberry
pushed a commit
to cadesalaberry/sentry-javascript
that referenced
this pull request
Apr 19, 2024
Next.js provides their own OTel http integration, which conflicts with ours ref getsentry#11016 added commit from this PR: getsentry#11319 --------- Co-authored-by: Luca Forstner <[email protected]> Co-authored-by: Francesco Novy <[email protected]>
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.
Next.js provides their own OTel http integration, which conflicts with ours
ref #11016
added commit from this PR: #11319