-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(bun): Add client address, port and protocol to Bun.serve spans #24523
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.
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
Changes from all commits
c569d01
d40a7e1
3db3557
6208449
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -21,8 +21,11 @@ import { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| filterCollectedUrl, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| filterCollectedUrlQuery, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from '@sentry/core'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { ServeOptions } from 'bun'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import type { Server, ServeOptions } from 'bun'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLIENT_ADDRESS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| CLIENT_PORT, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| NETWORK_PROTOCOL_NAME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| SENTRY_OP, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| SENTRY_SEGMENT_NAME_SOURCE, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| URL_DOMAIN, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -242,6 +245,24 @@ function wrapRequestHandler<T extends RouteHandler = RouteHandler>( | |||||||||||||||||||||||||||||||||||||||||||||||||||
| const client = getClient(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| const dataCollection = client?.getDataCollectionOptions(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (dataCollection?.userInfo) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // `client.address` is the originating client, so a forwarding header wins over the socket, which | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // behind a proxy holds the proxy's address. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| const forwardedFor = request.headers.get('x-forwarded-for')?.split(',')[0]?.trim(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Bun passes the `Server` as the second argument to both `fetch` and route handlers, except | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // when the handler runs through `server.fetch()`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| const socketAddress = getRequestIP(args[1], request); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (forwardedFor || socketAddress?.address) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| attributes[CLIENT_ADDRESS] = forwardedFor || socketAddress?.address; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (socketAddress?.port) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| attributes[CLIENT_PORT] = socketAddress.port; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+248
to
+261
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This section has an interesting bug (and Deno has the same one, which I'm guessing is how it got here). When
Suggested change
To be honest, it might be better to keep this PR focused on Bun, keep it consistent with Deno's (incorrect) behavior, and fix in a follow-up. But if you feel like updating it, we should get this and the similar fix applied to |
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // describes the OSI application-layer protocol (http), not the scheme (might be https) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| attributes[NETWORK_PROTOCOL_NAME] = 'http'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (dataCollection) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Object.assign(attributes, httpHeadersToSpanAttributes(request.headers.toJSON(), dataCollection)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -308,6 +329,18 @@ function wrapRequestHandler<T extends RouteHandler = RouteHandler>( | |||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| function getRequestIP(server: unknown, request: Request): { address: string; port: number } | undefined { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (typeof (server as Partial<Server> | undefined)?.requestIP !== 'function') { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return (server as Server).requestIP(request) ?? undefined; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Defensive: never let a failed lookup break the user's handler. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| function getSpanAttributesFromParsedUrl( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| parsedUrl: ReturnType<typeof parseStringToURLObject>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| request: Request, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(not a blocker or suggestion for this PR, making notes for follow-up)
The span takes the first
x-forwarded-forentry as-is. Core'sgetClientIPAddress(packages/core/src/vendor/getIpAddress.tsline 35), which setsuser.ip_addresson events, is different:isIP, so values likeunknownor junk are skipped. The span keeps them.Forwarded,X-Real-IP,CF-Connecting-IP,Fly-Client-IPand other headers. The span ignores them, so behind Cloudflare or nginx withX-Real-IPthe span reports the proxy while the event reports the client.So
client.addresson the span anduser.ip_addresson the event can disagree. Deno has the same code, and parity with Deno was the goal, so this is not a blocker.Suggestion (follow-up): export
getClientIPAddressfrom@sentry/coreand use it in both the Bun and Deno wrappers:getClientIPAddress(request.headers.toJSON()) || socketAddress?.address.