From 2c3066efd5c8047ceb48a4740afe6e043afe0713 Mon Sep 17 00:00:00 2001 From: Onur Temizkan Date: Thu, 13 Jul 2023 15:14:36 +0100 Subject: [PATCH 01/13] feat(remix): Add Remix v2 support (#8415) Adds support for new error handling utilities of Remix v2. ([ErrorBoundary](https://remix.run/docs/en/main/route/error-boundary-v2), [handleError](https://github.com/remix-run/remix/releases/tag/remix%401.17.0)) ## `ErrorBoundary` v2 Remix's `ErrorBoundary` captures all client / server / SSR errors and shows a customizable error page. In v1, to capture client-side errors we were wrapping the whole Remix application with `@sentry/react`s `ErrorBoundary` which caused inconsistencies in error pages. (See: https://github.com/getsentry/sentry-javascript/issues/5762) v2 implementation does not wrap user's application with `@sentry/react`'s ErrorBoundary, instead it exports a capturing utility to be used inside the Remix application's `ErrorBoundary` function. Can be used like: ```typescript import { captureRemixErrorBoundaryError } from '@sentry/remix'; export const ErrorBoundary: V2_ErrorBoundaryComponent = () => { const error = useRouteError(); captureRemixErrorBoundaryError(error); return
...
; }; ``` It also requires `v2_errorBoundary` [future flag](https://remix.run/docs/en/1.18.0/pages/api-development-strategy#current-future-flags) to be enabled. ## `handleError` For server-side errors apart from 'Error Responses' (thrown responses are handled in `ErrorBoundary`), this implementation exports another utility to be used in `handleError` function. The errors we capture in `handleError` also appear on `ErrorBoundary` functions but stacktraces are not available. So, we skip those errors in `captureRemixErrorBoundaryError` function. `handleError` can be instrumented as below: ```typescript export function handleError(error: unknown, { request }: DataFunctionArgs): void { if (error instanceof Error) { Sentry.captureRemixServerException(error, 'remix.server', request); } else { // Optionally Sentry.captureException(error); } ``` --- packages/remix/src/client/errors.tsx | 65 +++++++++++++++++++ .../client.tsx => client/performance.tsx} | 5 +- packages/remix/src/index.client.tsx | 3 +- packages/remix/src/index.server.ts | 4 +- packages/remix/src/utils/futureFlags.ts | 35 ++++++++++ packages/remix/src/utils/instrumentServer.ts | 30 +++++++-- .../remix/src/utils/serverAdapters/express.ts | 2 +- packages/remix/src/utils/vendor/response.ts | 2 +- .../remix/src/utils/{ => vendor}/types.ts | 37 +++++++++++ packages/remix/src/utils/web-fetch.ts | 2 +- .../test/integration/app_v2/entry.server.tsx | 10 ++- .../remix/test/integration/app_v2/root.tsx | 13 +++- .../routes/action-json-response.$id.tsx | 4 +- .../routes/loader-json-response.$id.tsx | 2 +- .../test/client/errorboundary.test.ts | 16 +++-- .../integration/test/server/action.test.ts | 22 ++++--- .../integration/test/server/loader.test.ts | 8 +-- 17 files changed, 225 insertions(+), 35 deletions(-) create mode 100644 packages/remix/src/client/errors.tsx rename packages/remix/src/{performance/client.tsx => client/performance.tsx} (95%) create mode 100644 packages/remix/src/utils/futureFlags.ts rename packages/remix/src/utils/{ => vendor}/types.ts (89%) diff --git a/packages/remix/src/client/errors.tsx b/packages/remix/src/client/errors.tsx new file mode 100644 index 000000000000..9c9fd5c4b449 --- /dev/null +++ b/packages/remix/src/client/errors.tsx @@ -0,0 +1,65 @@ +import { captureException, withScope } from '@sentry/core'; +import { addExceptionMechanism, isNodeEnv, isString } from '@sentry/utils'; + +import type { ErrorResponse } from '../utils/vendor/types'; + +/** + * Checks whether the given error is an ErrorResponse. + * ErrorResponse is when users throw a response from their loader or action functions. + * This is in fact a server-side error that we capture on the client. + * + * @param error The error to check. + * @returns boolean + */ +function isErrorResponse(error: unknown): error is ErrorResponse { + return typeof error === 'object' && error !== null && 'status' in error && 'statusText' in error; +} + +/** + * Captures an error that is thrown inside a Remix ErrorBoundary. + * + * @param error The error to capture. + * @returns void + */ +export function captureRemixErrorBoundaryError(error: unknown): void { + const isClientSideRuntimeError = !isNodeEnv() && error instanceof Error; + const isRemixErrorResponse = isErrorResponse(error); + // Server-side errors apart from `ErrorResponse`s also appear here without their stacktraces. + // So, we only capture: + // 1. `ErrorResponse`s + // 2. Client-side runtime errors here, + // And other server - side errors in `handleError` function where stacktraces are available. + if (isRemixErrorResponse || isClientSideRuntimeError) { + const eventData = isRemixErrorResponse + ? { + function: 'ErrorResponse', + ...error.data, + } + : { + function: 'ReactError', + }; + + withScope(scope => { + scope.addEventProcessor(event => { + addExceptionMechanism(event, { + type: 'instrument', + handled: true, + data: eventData, + }); + return event; + }); + + if (isRemixErrorResponse) { + if (isString(error.data)) { + captureException(error.data); + } else if (error.statusText) { + captureException(error.statusText); + } else { + captureException(error); + } + } else { + captureException(error); + } + }); + } +} diff --git a/packages/remix/src/performance/client.tsx b/packages/remix/src/client/performance.tsx similarity index 95% rename from packages/remix/src/performance/client.tsx rename to packages/remix/src/client/performance.tsx index 879c93e51f42..a3f7815b7bdc 100644 --- a/packages/remix/src/performance/client.tsx +++ b/packages/remix/src/client/performance.tsx @@ -4,6 +4,8 @@ import type { Transaction, TransactionContext } from '@sentry/types'; import { isNodeEnv, logger } from '@sentry/utils'; import * as React from 'react'; +import { getFutureFlagsBrowser } from '../utils/futureFlags'; + const DEFAULT_TAGS = { 'routing.instrumentation': 'remix-router', } as const; @@ -93,7 +95,8 @@ export function withSentry

, R extends React.FC wrapWithErrorBoundary?: boolean; errorBoundaryOptions?: ErrorBoundaryProps; } = { - wrapWithErrorBoundary: true, + // We don't want to wrap application with Sentry's ErrorBoundary by default for Remix v2 + wrapWithErrorBoundary: getFutureFlagsBrowser()?.v2_errorBoundary ? false : true, errorBoundaryOptions: {}, }, ): R { diff --git a/packages/remix/src/index.client.tsx b/packages/remix/src/index.client.tsx index 5c76ee4907bf..64951a3f10cd 100644 --- a/packages/remix/src/index.client.tsx +++ b/packages/remix/src/index.client.tsx @@ -3,7 +3,8 @@ import { configureScope, init as reactInit } from '@sentry/react'; import { buildMetadata } from './utils/metadata'; import type { RemixOptions } from './utils/remixOptions'; -export { remixRouterInstrumentation, withSentry } from './performance/client'; +export { remixRouterInstrumentation, withSentry } from './client/performance'; +export { captureRemixErrorBoundaryError } from './client/errors'; export * from '@sentry/react'; export function init(options: RemixOptions): void { diff --git a/packages/remix/src/index.server.ts b/packages/remix/src/index.server.ts index 91839a809993..4c37351001c2 100644 --- a/packages/remix/src/index.server.ts +++ b/packages/remix/src/index.server.ts @@ -54,8 +54,10 @@ export { // Keeping the `*` exports for backwards compatibility and types export * from '@sentry/node'; +export { captureRemixServerException } from './utils/instrumentServer'; export { ErrorBoundary, withErrorBoundary } from '@sentry/react'; -export { remixRouterInstrumentation, withSentry } from './performance/client'; +export { remixRouterInstrumentation, withSentry } from './client/performance'; +export { captureRemixErrorBoundaryError } from './client/errors'; export { wrapExpressCreateRequestHandler } from './utils/serverAdapters/express'; function sdkAlreadyInitialized(): boolean { diff --git a/packages/remix/src/utils/futureFlags.ts b/packages/remix/src/utils/futureFlags.ts new file mode 100644 index 000000000000..7d797c19e8a2 --- /dev/null +++ b/packages/remix/src/utils/futureFlags.ts @@ -0,0 +1,35 @@ +import { GLOBAL_OBJ } from '@sentry/utils'; + +import type { FutureConfig, ServerBuild } from './vendor/types'; + +export type EnhancedGlobal = typeof GLOBAL_OBJ & { + __remixContext?: { + future?: FutureConfig; + }; +}; + +/** + * Get the future flags from the Remix browser context + * + * @returns The future flags + */ +export function getFutureFlagsBrowser(): FutureConfig | undefined { + const window = GLOBAL_OBJ as EnhancedGlobal; + + if (!window.__remixContext) { + return; + } + + return window.__remixContext.future; +} + +/** + * Get the future flags from the Remix server build + * + * @param build The Remix server build + * + * @returns The future flags + */ +export function getFutureFlagsServer(build: ServerBuild): FutureConfig | undefined { + return build.future; +} diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 64dea4cfb92b..b0ba45f69c26 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/packages/remix/src/utils/instrumentServer.ts @@ -13,12 +13,15 @@ import { tracingContextFromHeaders, } from '@sentry/utils'; +import { getFutureFlagsServer } from './futureFlags'; +import { extractData, getRequestMatch, isDeferredData, isResponse, json, matchServerRoutes } from './vendor/response'; import type { AppData, CreateRequestHandlerFunction, DataFunction, DataFunctionArgs, EntryContext, + FutureConfig, HandleDocumentRequestFunction, ReactRouterDomPkg, RemixRequest, @@ -26,10 +29,11 @@ import type { ServerBuild, ServerRoute, ServerRouteManifest, -} from './types'; -import { extractData, getRequestMatch, isDeferredData, isResponse, json, matchServerRoutes } from './vendor/response'; +} from './vendor/types'; import { normalizeRemixRequest } from './web-fetch'; +let FUTURE_FLAGS: FutureConfig | undefined; + // Flag to track if the core request handler is instrumented. export let isRequestHandlerWrapped = false; @@ -56,7 +60,16 @@ async function extractResponseError(response: Response): Promise { return responseData; } -async function captureRemixServerException(err: unknown, name: string, request: Request): Promise { +/** + * Captures an exception happened in the Remix server. + * + * @param err The error to capture. + * @param name The name of the origin function. + * @param request The request object. + * + * @returns A promise that resolves when the exception is captured. + */ +export async function captureRemixServerException(err: unknown, name: string, request: Request): Promise { // Skip capturing if the thrown error is not a 5xx response // https://remix.run/docs/en/v1/api/conventions#throwing-responses-in-loaders if (isResponse(err) && err.status < 500) { @@ -145,7 +158,10 @@ function makeWrappedDocumentRequestFunction( span?.finish(); } catch (err) { - await captureRemixServerException(err, 'documentRequest', request); + if (!FUTURE_FLAGS?.v2_errorBoundary) { + await captureRemixServerException(err, 'documentRequest', request); + } + throw err; } @@ -182,7 +198,10 @@ function makeWrappedDataFunction(origFn: DataFunction, id: string, name: 'action currentScope.setSpan(activeTransaction); span?.finish(); } catch (err) { - await captureRemixServerException(err, name, args.request); + if (!FUTURE_FLAGS?.v2_errorBoundary) { + await captureRemixServerException(err, name, args.request); + } + throw err; } @@ -439,6 +458,7 @@ function makeWrappedCreateRequestHandler( isRequestHandlerWrapped = true; return function (this: unknown, build: ServerBuild, ...args: unknown[]): RequestHandler { + FUTURE_FLAGS = getFutureFlagsServer(build); const newBuild = instrumentBuild(build); const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args); diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index 000ad3a00b15..742c938f2d06 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/packages/remix/src/utils/serverAdapters/express.ts @@ -20,7 +20,7 @@ import type { ExpressResponse, ReactRouterDomPkg, ServerBuild, -} from '../types'; +} from '../vendor/types'; let pkg: ReactRouterDomPkg; diff --git a/packages/remix/src/utils/vendor/response.ts b/packages/remix/src/utils/vendor/response.ts index ae85fff74734..fed25dd0f534 100644 --- a/packages/remix/src/utils/vendor/response.ts +++ b/packages/remix/src/utils/vendor/response.ts @@ -6,7 +6,7 @@ // // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. -import type { DeferredData, ReactRouterDomPkg, RouteMatch, ServerRoute } from '../types'; +import type { DeferredData, ReactRouterDomPkg, RouteMatch, ServerRoute } from './types'; /** * Based on Remix Implementation diff --git a/packages/remix/src/utils/types.ts b/packages/remix/src/utils/vendor/types.ts similarity index 89% rename from packages/remix/src/utils/types.ts rename to packages/remix/src/utils/vendor/types.ts index 74dcf10215cc..faaa7e5f6f60 100644 --- a/packages/remix/src/utils/types.ts +++ b/packages/remix/src/utils/vendor/types.ts @@ -14,6 +14,42 @@ import type * as Express from 'express'; import type { Agent } from 'https'; import type { ComponentType } from 'react'; +type Dev = { + command?: string; + scheme?: string; + host?: string; + port?: number; + restart?: boolean; + tlsKey?: string; + tlsCert?: string; +}; + +export interface FutureConfig { + unstable_dev: boolean | Dev; + /** @deprecated Use the `postcss` config option instead */ + unstable_postcss: boolean; + /** @deprecated Use the `tailwind` config option instead */ + unstable_tailwind: boolean; + v2_errorBoundary: boolean; + v2_headers: boolean; + v2_meta: boolean; + v2_normalizeFormMethod: boolean; + v2_routeConvention: boolean; +} + +export interface RemixConfig { + [key: string]: any; + future: FutureConfig; +} + +export interface ErrorResponse { + status: number; + statusText: string; + data: any; + error?: Error; + internal: boolean; +} + export type RemixRequestState = { method: string; redirect: RequestRedirect; @@ -133,6 +169,7 @@ export interface ServerBuild { assets: AssetsManifest; publicPath?: string; assetsBuildDirectory?: string; + future?: FutureConfig; } export interface HandleDocumentRequestFunction { diff --git a/packages/remix/src/utils/web-fetch.ts b/packages/remix/src/utils/web-fetch.ts index 1e69a77b5dba..1961329c2f4b 100644 --- a/packages/remix/src/utils/web-fetch.ts +++ b/packages/remix/src/utils/web-fetch.ts @@ -24,8 +24,8 @@ import { logger } from '@sentry/utils'; -import type { RemixRequest } from './types'; import { getClientIPAddress } from './vendor/getIpAddress'; +import type { RemixRequest } from './vendor/types'; /* * Symbol extractor utility to be able to access internal fields of Remix requests. diff --git a/packages/remix/test/integration/app_v2/entry.server.tsx b/packages/remix/test/integration/app_v2/entry.server.tsx index d48f2644fac4..784cb2a39cd4 100644 --- a/packages/remix/test/integration/app_v2/entry.server.tsx +++ b/packages/remix/test/integration/app_v2/entry.server.tsx @@ -1,4 +1,4 @@ -import type { EntryContext } from '@remix-run/node'; +import type { EntryContext, DataFunctionArgs } from '@remix-run/node'; import { RemixServer } from '@remix-run/react'; import { renderToString } from 'react-dom/server'; import * as Sentry from '@sentry/remix'; @@ -11,6 +11,14 @@ Sentry.init({ autoSessionTracking: false, }); +export function handleError(error: unknown, { request }: DataFunctionArgs): void { + if (error instanceof Error) { + Sentry.captureRemixServerException(error, 'remix.server', request); + } else { + Sentry.captureException(error); + } +} + export default function handleRequest( request: Request, responseStatusCode: number, diff --git a/packages/remix/test/integration/app_v2/root.tsx b/packages/remix/test/integration/app_v2/root.tsx index 5af1f8cc7a1a..2320451cee74 100644 --- a/packages/remix/test/integration/app_v2/root.tsx +++ b/packages/remix/test/integration/app_v2/root.tsx @@ -1,6 +1,15 @@ import { V2_MetaFunction, LoaderFunction, json, defer, redirect } from '@remix-run/node'; -import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration } from '@remix-run/react'; -import { withSentry } from '@sentry/remix'; +import { Links, LiveReload, Meta, Outlet, Scripts, ScrollRestoration, useRouteError } from '@remix-run/react'; +import { V2_ErrorBoundaryComponent } from '@remix-run/react/dist/routeModules'; +import { captureRemixErrorBoundaryError, withSentry } from '@sentry/remix'; + +export const ErrorBoundary: V2_ErrorBoundaryComponent = () => { + const error = useRouteError(); + + captureRemixErrorBoundaryError(error); + + return

error
; +}; export const meta: V2_MetaFunction = ({ data }) => [ { charset: 'utf-8' }, diff --git a/packages/remix/test/integration/common/routes/action-json-response.$id.tsx b/packages/remix/test/integration/common/routes/action-json-response.$id.tsx index 2c7a19059596..ff0f6940fe44 100644 --- a/packages/remix/test/integration/common/routes/action-json-response.$id.tsx +++ b/packages/remix/test/integration/common/routes/action-json-response.$id.tsx @@ -3,8 +3,10 @@ import { useActionData } from '@remix-run/react'; export const loader: LoaderFunction = async ({ params: { id } }) => { if (id === '-1') { - throw new Error('Unexpected Server Error from Loader'); + throw new Error('Unexpected Server Error'); } + + return null; }; export const action: ActionFunction = async ({ params: { id } }) => { diff --git a/packages/remix/test/integration/common/routes/loader-json-response.$id.tsx b/packages/remix/test/integration/common/routes/loader-json-response.$id.tsx index 55b53e2d70dc..a4ad3dc48339 100644 --- a/packages/remix/test/integration/common/routes/loader-json-response.$id.tsx +++ b/packages/remix/test/integration/common/routes/loader-json-response.$id.tsx @@ -5,7 +5,7 @@ type LoaderData = { id: string }; export const loader: LoaderFunction = async ({ params: { id } }) => { if (id === '-2') { - throw new Error('Unexpected Server Error from Loader'); + throw new Error('Unexpected Server Error'); } if (id === '-1') { diff --git a/packages/remix/test/integration/test/client/errorboundary.test.ts b/packages/remix/test/integration/test/client/errorboundary.test.ts index b90b3e8d3eaa..9c496e3e3040 100644 --- a/packages/remix/test/integration/test/client/errorboundary.test.ts +++ b/packages/remix/test/integration/test/client/errorboundary.test.ts @@ -21,16 +21,20 @@ test('should capture React component errors.', async ({ page }) => { expect(errorEnvelope.level).toBe('error'); expect(errorEnvelope.sdk?.name).toBe('sentry.javascript.remix'); expect(errorEnvelope.exception?.values).toMatchObject([ - { - type: 'React ErrorBoundary Error', - value: 'Sentry React Component Error', - stacktrace: { frames: expect.any(Array) }, - }, + ...(!useV2 + ? [ + { + type: 'React ErrorBoundary Error', + value: 'Sentry React Component Error', + stacktrace: { frames: expect.any(Array) }, + }, + ] + : []), { type: 'Error', value: 'Sentry React Component Error', stacktrace: { frames: expect.any(Array) }, - mechanism: { type: 'generic', handled: true }, + mechanism: { type: useV2 ? 'instrument' : 'generic', handled: true }, }, ]); }); diff --git a/packages/remix/test/integration/test/server/action.test.ts b/packages/remix/test/integration/test/server/action.test.ts index cc25a87611d4..a2a4632ba962 100644 --- a/packages/remix/test/integration/test/server/action.test.ts +++ b/packages/remix/test/integration/test/server/action.test.ts @@ -81,7 +81,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'remix.server' : 'action', }, handled: true, type: 'instrument', @@ -197,11 +197,11 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada values: [ { type: 'Error', - value: 'Unexpected Server Error from Loader', + value: 'Unexpected Server Error', stacktrace: expect.any(Object), mechanism: { data: { - function: 'loader', + function: useV2 ? 'remix.server' : 'loader', }, handled: true, type: 'instrument', @@ -254,7 +254,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'ErrorResponse' : 'action', }, handled: true, type: 'instrument', @@ -303,11 +303,13 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada values: [ { type: 'Error', - value: 'Non-Error exception captured with keys: data', + value: useV2 + ? 'Non-Error exception captured with keys: data, internal, status, statusText' + : 'Non-Error exception captured with keys: data', stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'ErrorResponse' : 'action', }, handled: true, type: 'instrument', @@ -360,7 +362,7 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'ErrorResponse' : 'action', }, handled: true, type: 'instrument', @@ -409,11 +411,13 @@ describe.each(['builtin', 'express'])('Remix API Actions with adapter = %s', ada values: [ { type: 'Error', - value: 'Non-Error exception captured with keys: [object has no keys]', + value: useV2 + ? 'Non-Error exception captured with keys: data, internal, status, statusText' + : 'Non-Error exception captured with keys: [object has no keys]', stacktrace: expect.any(Object), mechanism: { data: { - function: 'action', + function: useV2 ? 'ErrorResponse' : 'action', }, handled: true, type: 'instrument', diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 8a99c699cc37..ccaa93b05e36 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/packages/remix/test/integration/test/server/loader.test.ts @@ -35,11 +35,11 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada values: [ { type: 'Error', - value: 'Unexpected Server Error from Loader', + value: 'Unexpected Server Error', stacktrace: expect.any(Object), mechanism: { data: { - function: 'loader', + function: useV2 ? 'remix.server' : 'loader', }, handled: true, type: 'instrument', @@ -134,11 +134,11 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada values: [ { type: 'Error', - value: 'Unexpected Server Error from Loader', + value: 'Unexpected Server Error', stacktrace: expect.any(Object), mechanism: { data: { - function: 'loader', + function: useV2 ? 'remix.server' : 'loader', }, handled: true, type: 'instrument', From 2b4121fad0e04724619de62d656e6323b8289f1f Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Thu, 13 Jul 2023 19:09:19 +0100 Subject: [PATCH 02/13] feat(core): Add `ModuleMetadata` integration (#8475) - Adds the `ModuleMetadata` integration that fetches metadata injected via bundler plugins and attaches is to the `module_metadata` property of every `StackFrame`. - This can later be used in `beforeSend` or another integration to route events depending on the metadata. - This integration is - Exported separately from `@sentry/core` (ie. not in `Integrations`) so it doesn't get included in default bundles - Exported separately from `@sentry/browser` so that it can be used without depending directly on core - Uses the `beforeEnvelope` hook to strip the `module_metadata` property from stack frames - Adds a test to ensure `module_metadata` is available in `beforeSend` and is stripped before sending --- packages/browser/src/index.ts | 1 + packages/core/src/index.ts | 2 +- packages/core/src/integrations/metadata.ts | 57 ++++++++++++++++ .../test/lib/integrations/metadata.test.ts | 66 +++++++++++++++++++ packages/core/test/mocks/client.ts | 11 +++- 5 files changed, 134 insertions(+), 3 deletions(-) create mode 100644 packages/core/src/integrations/metadata.ts create mode 100644 packages/core/test/lib/integrations/metadata.test.ts diff --git a/packages/browser/src/index.ts b/packages/browser/src/index.ts index 0a69fc44d858..9dbe7f977d7e 100644 --- a/packages/browser/src/index.ts +++ b/packages/browser/src/index.ts @@ -34,6 +34,7 @@ export { spanStatusfromHttpCode, trace, makeMultiplexedTransport, + ModuleMetadata, } from '@sentry/core'; export type { SpanStatusType } from '@sentry/core'; export type { Span } from '@sentry/types'; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index db9d21c2f2e8..9ff61f05cdb3 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -46,7 +46,7 @@ export { prepareEvent } from './utils/prepareEvent'; export { createCheckInEnvelope } from './checkin'; export { hasTracingEnabled } from './utils/hasTracingEnabled'; export { DEFAULT_ENVIRONMENT } from './constants'; - +export { ModuleMetadata } from './integrations/metadata'; import * as Integrations from './integrations'; export { Integrations }; diff --git a/packages/core/src/integrations/metadata.ts b/packages/core/src/integrations/metadata.ts new file mode 100644 index 000000000000..05af1d88ebe9 --- /dev/null +++ b/packages/core/src/integrations/metadata.ts @@ -0,0 +1,57 @@ +import type { EventItem, EventProcessor, Hub, Integration } from '@sentry/types'; +import { forEachEnvelopeItem } from '@sentry/utils'; + +import { addMetadataToStackFrames, stripMetadataFromStackFrames } from '../metadata'; + +/** + * Adds module metadata to stack frames. + * + * Metadata can be injected by the Sentry bundler plugins using the `_experiments.moduleMetadata` config option. + * + * When this integration is added, the metadata passed to the bundler plugin is added to the stack frames of all events + * under the `module_metadata` property. This can be used to help in tagging or routing of events from different teams + * our sources + */ +export class ModuleMetadata implements Integration { + /* + * @inheritDoc + */ + public static id: string = 'ModuleMetadata'; + + /** + * @inheritDoc + */ + public name: string = ModuleMetadata.id; + + /** + * @inheritDoc + */ + public setupOnce(addGlobalEventProcessor: (processor: EventProcessor) => void, getCurrentHub: () => Hub): void { + const client = getCurrentHub().getClient(); + + if (!client || typeof client.on !== 'function') { + return; + } + + // We need to strip metadata from stack frames before sending them to Sentry since these are client side only. + client.on('beforeEnvelope', envelope => { + forEachEnvelopeItem(envelope, (item, type) => { + if (type === 'event') { + const event = Array.isArray(item) ? (item as EventItem)[1] : undefined; + + if (event) { + stripMetadataFromStackFrames(event); + item[1] = event; + } + } + }); + }); + + const stackParser = client.getOptions().stackParser; + + addGlobalEventProcessor(event => { + addMetadataToStackFrames(stackParser, event); + return event; + }); + } +} diff --git a/packages/core/test/lib/integrations/metadata.test.ts b/packages/core/test/lib/integrations/metadata.test.ts new file mode 100644 index 000000000000..464da2d97fbb --- /dev/null +++ b/packages/core/test/lib/integrations/metadata.test.ts @@ -0,0 +1,66 @@ +import type { Event } from '@sentry/types'; +import { createStackParser, GLOBAL_OBJ, nodeStackLineParser, parseEnvelope } from '@sentry/utils'; +import { TextDecoder, TextEncoder } from 'util'; + +import { createTransport, getCurrentHub, ModuleMetadata } from '../../../src'; +import { getDefaultTestClientOptions, TestClient } from '../../mocks/client'; + +const stackParser = createStackParser(nodeStackLineParser()); + +const stack = new Error().stack || ''; + +describe('ModuleMetadata integration', () => { + beforeEach(() => { + TestClient.sendEventCalled = undefined; + TestClient.instance = undefined; + + GLOBAL_OBJ._sentryModuleMetadata = GLOBAL_OBJ._sentryModuleMetadata || {}; + GLOBAL_OBJ._sentryModuleMetadata[stack] = { team: 'frontend' }; + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + test('Adds and removes metadata from stack frames', done => { + const options = getDefaultTestClientOptions({ + dsn: 'https://username@domain/123', + enableSend: true, + stackParser, + integrations: [new ModuleMetadata()], + beforeSend: (event, _hint) => { + // copy the frames since reverse in in-place + const lastFrame = [...(event.exception?.values?.[0].stacktrace?.frames || [])].reverse()[0]; + // Ensure module_metadata is populated in beforeSend callback + expect(lastFrame?.module_metadata).toEqual({ team: 'frontend' }); + return event; + }, + transport: () => + createTransport({ recordDroppedEvent: () => undefined, textEncoder: new TextEncoder() }, async req => { + const [, items] = parseEnvelope(req.body, new TextEncoder(), new TextDecoder()); + + expect(items[0][1]).toBeDefined(); + const event = items[0][1] as Event; + const error = event.exception?.values?.[0]; + + // Ensure we're looking at the same error we threw + expect(error?.value).toEqual('Some error'); + + const lastFrame = [...(error?.stacktrace?.frames || [])].reverse()[0]; + // Ensure the last frame is in fact for this file + expect(lastFrame?.filename).toEqual(__filename); + + // Ensure module_metadata has been stripped from the event + expect(lastFrame?.module_metadata).toBeUndefined(); + + done(); + return {}; + }), + }); + + const client = new TestClient(options); + const hub = getCurrentHub(); + hub.bindClient(client); + hub.captureException(new Error('Some error')); + }); +}); diff --git a/packages/core/test/mocks/client.ts b/packages/core/test/mocks/client.ts index 7ca980189e19..f7d8830a1402 100644 --- a/packages/core/test/mocks/client.ts +++ b/packages/core/test/mocks/client.ts @@ -54,7 +54,7 @@ export class TestClient extends BaseClient { // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types public eventFromException(exception: any): PromiseLike { - return resolvedSyncPromise({ + const event: Event = { exception: { values: [ { @@ -65,7 +65,14 @@ export class TestClient extends BaseClient { }, ], }, - }); + }; + + const frames = this._options.stackParser(exception.stack || '', 1); + if (frames.length && event?.exception?.values?.[0]) { + event.exception.values[0] = { ...event.exception.values[0], stacktrace: { frames } }; + } + + return resolvedSyncPromise(event); } public eventFromMessage( From 69a4fa325a77ebcbc6e2e77955637a4b0a08e16a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 17 Jul 2023 13:38:14 +0200 Subject: [PATCH 03/13] test(nextjs): Pin Nextjs 13 integration tests to `next@13.4.9` (#8551) As long as our SDK and `next@13.4.10` aren't compatible, we need to hard-pin our NextJS integration tests to `13.4.9` to unblock our CI. We need to revert this change once we figured out 13.4.10! (Set a reminder for myself) --- packages/nextjs/test/run-integration-tests.sh | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/test/run-integration-tests.sh b/packages/nextjs/test/run-integration-tests.sh index 0eff88612e95..a5d42e527f07 100755 --- a/packages/nextjs/test/run-integration-tests.sh +++ b/packages/nextjs/test/run-integration-tests.sh @@ -67,13 +67,21 @@ for NEXTJS_VERSION in 10 11 12 13; do rm -rf node_modules .next .env.local 2>/dev/null || true echo "[nextjs@$NEXTJS_VERSION] Installing dependencies..." + # set the desired version of next long enough to run yarn, and then restore the old version (doing the restoration now # rather than during overall cleanup lets us look for "latest" in every loop) + + if [ "$NEXTJS_VERSION" -eq "13" ]; then + SPECIFIC_NEXT_VERSION="13.4.9" + else + SPECIFIC_NEXT_VERSION="$NEXTJS_VERSION.x" + fi + cp package.json package.json.bak if [[ $(uname) == "Darwin" ]]; then - sed -i "" /"next.*latest"/s/latest/"${NEXTJS_VERSION}.x"/ package.json + sed -i "" /"next.*latest"/s/latest/"${SPECIFIC_NEXT_VERSION}"/ package.json else - sed -i /"next.*latest"/s/latest/"${NEXTJS_VERSION}.x"/ package.json + sed -i /"next.*latest"/s/latest/"${SPECIFIC_NEXT_VERSION}"/ package.json fi # Next.js v13 requires React 18.2.0 From c2b1bfdbd7db4ef036333c9750cdf134991b8a61 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 17 Jul 2023 14:04:01 +0200 Subject: [PATCH 04/13] fix(replay): Handle errors in `beforeAddRecordingEvent` callback (#8548) When an error occurs in `beforeAddRecordingEvent`, we just skip this event and log the error. Closes https://github.com/getsentry/sentry-javascript/issues/8542 --- packages/replay/src/util/addEvent.ts | 24 +++++++-- .../beforeAddRecordingEvent.test.ts | 51 ++++++++++++++++--- 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/packages/replay/src/util/addEvent.ts b/packages/replay/src/util/addEvent.ts index 9533c1690dd8..fdc755ada91c 100644 --- a/packages/replay/src/util/addEvent.ts +++ b/packages/replay/src/util/addEvent.ts @@ -3,7 +3,7 @@ import { getCurrentHub } from '@sentry/core'; import { logger } from '@sentry/utils'; import { EventBufferSizeExceededError } from '../eventBuffer/error'; -import type { AddEventResult, RecordingEvent, ReplayContainer, ReplayFrameEvent } from '../types'; +import type { AddEventResult, RecordingEvent, ReplayContainer, ReplayFrameEvent, ReplayPluginOptions } from '../types'; import { timestampToMs } from './timestampToMs'; function isCustomEvent(event: RecordingEvent): event is ReplayFrameEvent { @@ -46,10 +46,7 @@ export async function addEvent( const replayOptions = replay.getOptions(); - const eventAfterPossibleCallback = - typeof replayOptions.beforeAddRecordingEvent === 'function' && isCustomEvent(event) - ? replayOptions.beforeAddRecordingEvent(event) - : event; + const eventAfterPossibleCallback = maybeApplyCallback(event, replayOptions.beforeAddRecordingEvent); if (!eventAfterPossibleCallback) { return; @@ -69,3 +66,20 @@ export async function addEvent( } } } + +function maybeApplyCallback( + event: RecordingEvent, + callback: ReplayPluginOptions['beforeAddRecordingEvent'], +): RecordingEvent | null | undefined { + try { + if (typeof callback === 'function' && isCustomEvent(event)) { + return callback(event); + } + } catch (error) { + __DEBUG_BUILD__ && + logger.error('[Replay] An error occured in the `beforeAddRecordingEvent` callback, skipping the event...', error); + return null; + } + + return event; +} diff --git a/packages/replay/test/integration/beforeAddRecordingEvent.test.ts b/packages/replay/test/integration/beforeAddRecordingEvent.test.ts index af552e6104d8..6bf33f182e66 100644 --- a/packages/replay/test/integration/beforeAddRecordingEvent.test.ts +++ b/packages/replay/test/integration/beforeAddRecordingEvent.test.ts @@ -5,7 +5,8 @@ import * as SentryUtils from '@sentry/utils'; import type { Replay } from '../../src'; import type { ReplayContainer } from '../../src/replay'; import { clearSession } from '../../src/session/clearSession'; -import type { EventType } from '../../src/types'; +import { createPerformanceEntries } from '../../src/util/createPerformanceEntries'; +import { createPerformanceSpans } from '../../src/util/createPerformanceSpans'; import * as SendReplayRequest from '../../src/util/sendReplayRequest'; import { BASE_TIMESTAMP, mockRrweb, mockSdk } from '../index'; import { useFakeTimers } from '../utils/use-fake-timers'; @@ -40,6 +41,10 @@ describe('Integration | beforeAddRecordingEvent', () => { beforeAddRecordingEvent: event => { const eventData = event.data; + if (eventData.tag === 'performanceSpan') { + throw new Error('test error in callback'); + } + if (eventData.tag === 'breadcrumb' && eventData.payload.category === 'ui.click') { return { ...event, @@ -53,12 +58,6 @@ describe('Integration | beforeAddRecordingEvent', () => { }; } - // This should not do anything because callback should not be called - // for `event.type != 5` - but we guard anyhow to be safe - if ((event.type as EventType) === 2) { - return null; - } - if (eventData.tag === 'options') { return null; } @@ -143,4 +142,42 @@ describe('Integration | beforeAddRecordingEvent', () => { recordingData: JSON.stringify([{ data: { isCheckout: true }, timestamp: BASE_TIMESTAMP, type: 2 }]), }); }); + + it('handles error in callback', async () => { + createPerformanceSpans( + replay, + createPerformanceEntries([ + { + name: 'https://sentry.io/foo.js', + entryType: 'resource', + startTime: 176.59999990463257, + duration: 5.600000023841858, + initiatorType: 'link', + nextHopProtocol: 'h2', + workerStart: 177.5, + redirectStart: 0, + redirectEnd: 0, + fetchStart: 177.69999992847443, + domainLookupStart: 177.69999992847443, + domainLookupEnd: 177.69999992847443, + connectStart: 177.69999992847443, + connectEnd: 177.69999992847443, + secureConnectionStart: 177.69999992847443, + requestStart: 177.5, + responseStart: 181, + responseEnd: 182.19999992847443, + transferSize: 0, + encodedBodySize: 0, + decodedBodySize: 0, + serverTiming: [], + } as unknown as PerformanceResourceTiming, + ]), + ); + + jest.runAllTimers(); + await new Promise(process.nextTick); + + expect(replay).not.toHaveLastSentReplay(); + expect(replay.isEnabled()).toBe(true); + }); }); From 6444b3403952769f7a3f56f54c3e6628b1bf30dc Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 17 Jul 2023 14:12:53 +0200 Subject: [PATCH 05/13] fix(replay): Better session storage check (#8547) Apparently accessing sessionStorage in an iframe with certain permissions can result in a throw, so we try-catch this to ensure we do not produce any errors. Closes https://github.com/getsentry/sentry-javascript/issues/8392 --- packages/replay/src/util/hasSessionStorage.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/replay/src/util/hasSessionStorage.ts b/packages/replay/src/util/hasSessionStorage.ts index f242df101c25..e7be6788eb1a 100644 --- a/packages/replay/src/util/hasSessionStorage.ts +++ b/packages/replay/src/util/hasSessionStorage.ts @@ -2,5 +2,10 @@ import { WINDOW } from '../constants'; /** If sessionStorage is available. */ export function hasSessionStorage(): boolean { - return 'sessionStorage' in WINDOW && !!WINDOW.sessionStorage; + try { + // This can throw, e.g. when being accessed in a sandboxed iframe + return 'sessionStorage' in WINDOW && !!WINDOW.sessionStorage; + } catch { + return false; + } } From 459c5d2c765575716e63ad043e087eefb558d6ae Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 17 Jul 2023 15:03:41 +0200 Subject: [PATCH 06/13] build: Ensure `build:dev:watch` commands work (#8553) Noticed the commands are a bit off, so fixed them! --- packages/browser/package.json | 2 +- packages/integration-shims/package.json | 2 +- packages/replay-worker/package.json | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/browser/package.json b/packages/browser/package.json index f4d458cd56c6..8a19c7880993 100644 --- a/packages/browser/package.json +++ b/packages/browser/package.json @@ -64,7 +64,7 @@ "build:types:core": "tsc -p tsconfig.types.json", "build:types:downlevel": "yarn downlevel-dts build/npm/types build/npm/types-ts3.8 --to ts3.8", "build:watch": "run-p build:transpile:watch build:bundle:watch build:types:watch", - "build:dev:watch": "yarn build:watch", + "build:dev:watch": "run-p build:transpile:watch build:types:watch", "build:bundle:watch": "rollup -c rollup.bundle.config.js --watch", "build:transpile:watch": "rollup -c rollup.npm.config.js --watch", "build:types:watch": "tsc -p tsconfig.types.json --watch", diff --git a/packages/integration-shims/package.json b/packages/integration-shims/package.json index d1c107a217d1..27ad8574c5fe 100644 --- a/packages/integration-shims/package.json +++ b/packages/integration-shims/package.json @@ -24,7 +24,7 @@ "build:watch": "run-p build:transpile:watch build:types:watch", "build:dev:watch": "run-p build:watch", "build:transpile:watch": "yarn build:transpile --watch", - "build:types:watch": "yarn build:types --watch", + "build:types:watch": "tsc -p tsconfig.types.json --watch", "clean": "rimraf build", "fix": "run-s fix:eslint fix:prettier", "fix:eslint": "eslint . --format stylish --fix", diff --git a/packages/replay-worker/package.json b/packages/replay-worker/package.json index 137a5e58c96a..2d1623dd8d20 100644 --- a/packages/replay-worker/package.json +++ b/packages/replay-worker/package.json @@ -22,9 +22,9 @@ "build:types:downlevel": "yarn downlevel-dts build/npm/types build/npm/types-ts3.8 --to ts3.8", "build:dev": "yarn build", "build:watch": "run-p build:transpile:watch build:types:watch", - "build:dev:watch": "run-p build:watch", + "build:dev:watch": "yarn build:watch", "build:transpile:watch": "yarn build:transpile --watch", - "build:types:watch": "yarn build:types --watch", + "build:types:watch": "tsc -p tsconfig.types.json --watch", "clean": "rimraf build", "fix": "run-s fix:eslint fix:prettier", "fix:eslint": "eslint . --format stylish --fix", From 29195118575729b6bddbf1dd8f26bf30ecab9a63 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Mon, 17 Jul 2023 17:28:49 +0200 Subject: [PATCH 07/13] fix(otel): Use `HTTP_URL` attribute for client requests (#8539) Turns out, `HTTP_TARGET` is always the relative path, even for outgoing requests, which omits the host. So we handle this specifically now here. While at it (and as I'm working a bit in this codebase), I also renamed the files from kebap-case to camelCase, to align with the rest of the codebase better - unless there was a specific reason to use that here @AbhiPrasad ? Closes https://github.com/getsentry/sentry-javascript/issues/8535 --- .../opentelemetry-node/src/spanprocessor.ts | 26 +++- ...s-sentry-request.ts => isSentryRequest.ts} | 0 .../{map-otel-status.ts => mapOtelStatus.ts} | 0 ...ription.ts => parseOtelSpanDescription.ts} | 76 +++++++-- .../test/spanprocessor.test.ts | 48 +++++- .../utils/parseOtelSpanDescription.test.ts | 144 ++++++++++++++++++ 6 files changed, 279 insertions(+), 15 deletions(-) rename packages/opentelemetry-node/src/utils/{is-sentry-request.ts => isSentryRequest.ts} (100%) rename packages/opentelemetry-node/src/utils/{map-otel-status.ts => mapOtelStatus.ts} (100%) rename packages/opentelemetry-node/src/utils/{parse-otel-span-description.ts => parseOtelSpanDescription.ts} (58%) create mode 100644 packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts diff --git a/packages/opentelemetry-node/src/spanprocessor.ts b/packages/opentelemetry-node/src/spanprocessor.ts index d6719fd682ad..980084a9adec 100644 --- a/packages/opentelemetry-node/src/spanprocessor.ts +++ b/packages/opentelemetry-node/src/spanprocessor.ts @@ -7,9 +7,9 @@ import type { DynamicSamplingContext, Span as SentrySpan, TraceparentData, Trans import { isString, logger } from '@sentry/utils'; import { SENTRY_DYNAMIC_SAMPLING_CONTEXT_KEY, SENTRY_TRACE_PARENT_CONTEXT_KEY } from './constants'; -import { isSentryRequestSpan } from './utils/is-sentry-request'; -import { mapOtelStatus } from './utils/map-otel-status'; -import { parseSpanDescription } from './utils/parse-otel-span-description'; +import { isSentryRequestSpan } from './utils/isSentryRequest'; +import { mapOtelStatus } from './utils/mapOtelStatus'; +import { parseSpanDescription } from './utils/parseOtelSpanDescription'; export const SENTRY_SPAN_PROCESSOR_MAP: Map = new Map< SentrySpan['spanId'], @@ -207,6 +207,8 @@ function getTraceData(otelSpan: OtelSpan, parentContext: Context): Partial { + const value = data[prop]; + sentrySpan.setData(prop, value); + }); + } + sentrySpan.op = op; sentrySpan.description = description; } function updateTransactionWithOtelData(transaction: Transaction, otelSpan: OtelSpan): void { + const { op, description, source, data } = parseSpanDescription(otelSpan); + transaction.setContext('otel', { attributes: otelSpan.attributes, resource: otelSpan.resource.attributes, }); + if (data) { + Object.keys(data).forEach(prop => { + const value = data[prop]; + transaction.setData(prop, value); + }); + } + transaction.setStatus(mapOtelStatus(otelSpan)); - const { op, description, source } = parseSpanDescription(otelSpan); transaction.op = op; transaction.setName(description, source); } diff --git a/packages/opentelemetry-node/src/utils/is-sentry-request.ts b/packages/opentelemetry-node/src/utils/isSentryRequest.ts similarity index 100% rename from packages/opentelemetry-node/src/utils/is-sentry-request.ts rename to packages/opentelemetry-node/src/utils/isSentryRequest.ts diff --git a/packages/opentelemetry-node/src/utils/map-otel-status.ts b/packages/opentelemetry-node/src/utils/mapOtelStatus.ts similarity index 100% rename from packages/opentelemetry-node/src/utils/map-otel-status.ts rename to packages/opentelemetry-node/src/utils/mapOtelStatus.ts diff --git a/packages/opentelemetry-node/src/utils/parse-otel-span-description.ts b/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts similarity index 58% rename from packages/opentelemetry-node/src/utils/parse-otel-span-description.ts rename to packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts index 396e37a29ced..bfef8a4a5885 100644 --- a/packages/opentelemetry-node/src/utils/parse-otel-span-description.ts +++ b/packages/opentelemetry-node/src/utils/parseOtelSpanDescription.ts @@ -1,13 +1,15 @@ -import type { AttributeValue } from '@opentelemetry/api'; +import type { Attributes, AttributeValue } from '@opentelemetry/api'; import { SpanKind } from '@opentelemetry/api'; import type { Span as OtelSpan } from '@opentelemetry/sdk-trace-base'; import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; import type { TransactionSource } from '@sentry/types'; +import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils'; interface SpanDescription { op: string | undefined; description: string; source: TransactionSource; + data?: Record; } /** @@ -87,21 +89,77 @@ function descriptionForHttpMethod(otelSpan: OtelSpan, httpMethod: AttributeValue break; } - const httpTarget = attributes[SemanticAttributes.HTTP_TARGET]; const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE]; + const { urlPath, url, query, fragment } = getSanitizedUrl(attributes, kind); - // Ex. /api/users - const httpPath = httpRoute || httpTarget; - - if (!httpPath) { + if (!urlPath) { return { op: opParts.join('.'), description: name, source: 'custom' }; } // Ex. description="GET /api/users". - const description = `${httpMethod} ${httpPath}`; + const description = `${httpMethod} ${urlPath}`; // If `httpPath` is a root path, then we can categorize the transaction source as route. - const source: TransactionSource = httpRoute || httpPath === '/' ? 'route' : 'url'; + const source: TransactionSource = httpRoute || urlPath === '/' ? 'route' : 'url'; + + const data: Record = {}; + + if (url) { + data.url = url; + } + if (query) { + data['http.query'] = query; + } + if (fragment) { + data['http.fragment'] = fragment; + } + + return { + op: opParts.join('.'), + description, + source, + data, + }; +} + +/** Exported for tests only */ +export function getSanitizedUrl( + attributes: Attributes, + kind: SpanKind, +): { + url: string | undefined; + urlPath: string | undefined; + query: string | undefined; + fragment: string | undefined; +} { + // This is the relative path of the URL, e.g. /sub + const httpTarget = attributes[SemanticAttributes.HTTP_TARGET]; + // This is the full URL, including host & query params etc., e.g. https://example.com/sub?foo=bar + const httpUrl = attributes[SemanticAttributes.HTTP_URL]; + // This is the normalized route name - may not always be available! + const httpRoute = attributes[SemanticAttributes.HTTP_ROUTE]; + + const parsedUrl = typeof httpUrl === 'string' ? parseUrl(httpUrl) : undefined; + const url = parsedUrl ? getSanitizedUrlString(parsedUrl) : undefined; + const query = parsedUrl && parsedUrl.search ? parsedUrl.search : undefined; + const fragment = parsedUrl && parsedUrl.hash ? parsedUrl.hash : undefined; + + if (typeof httpRoute === 'string') { + return { urlPath: httpRoute, url, query, fragment }; + } + + if (kind === SpanKind.SERVER && typeof httpTarget === 'string') { + return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment }; + } + + if (parsedUrl) { + return { urlPath: url, url, query, fragment }; + } + + // fall back to target even for client spans, if no URL is present + if (typeof httpTarget === 'string') { + return { urlPath: stripUrlQueryAndFragment(httpTarget), url, query, fragment }; + } - return { op: opParts.join('.'), description, source }; + return { urlPath: undefined, url, query, fragment }; } diff --git a/packages/opentelemetry-node/test/spanprocessor.test.ts b/packages/opentelemetry-node/test/spanprocessor.test.ts index cde0c7338d00..38c72ef5d3b8 100644 --- a/packages/opentelemetry-node/test/spanprocessor.test.ts +++ b/packages/opentelemetry-node/test/spanprocessor.test.ts @@ -434,10 +434,19 @@ describe('SentrySpanProcessor', () => { child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET'); child.setAttribute(SemanticAttributes.HTTP_ROUTE, '/my/route/{id}'); child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123'); + child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123'); child.end(); expect(sentrySpan?.description).toBe('GET /my/route/{id}'); + expect(sentrySpan?.data).toEqual({ + 'http.method': 'GET', + 'http.route': '/my/route/{id}', + 'http.target': '/my/route/123', + 'http.url': 'http://example.com/my/route/123', + 'otel.kind': 'INTERNAL', + url: 'http://example.com/my/route/123', + }); parentOtelSpan.end(); }); @@ -453,10 +462,47 @@ describe('SentrySpanProcessor', () => { child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET'); child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123'); + child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123'); child.end(); - expect(sentrySpan?.description).toBe('GET /my/route/123'); + expect(sentrySpan?.description).toBe('GET http://example.com/my/route/123'); + expect(sentrySpan?.data).toEqual({ + 'http.method': 'GET', + 'http.target': '/my/route/123', + 'http.url': 'http://example.com/my/route/123', + 'otel.kind': 'INTERNAL', + url: 'http://example.com/my/route/123', + }); + + parentOtelSpan.end(); + }); + }); + }); + + it('Adds query & hash data based on HTTP_URL', async () => { + const tracer = provider.getTracer('default'); + + tracer.startActiveSpan('GET /users', parentOtelSpan => { + tracer.startActiveSpan('HTTP GET', child => { + const sentrySpan = getSpanForOtelSpan(child); + + child.setAttribute(SemanticAttributes.HTTP_METHOD, 'GET'); + child.setAttribute(SemanticAttributes.HTTP_TARGET, '/my/route/123'); + child.setAttribute(SemanticAttributes.HTTP_URL, 'http://example.com/my/route/123?what=123#myHash'); + + child.end(); + + expect(sentrySpan?.description).toBe('GET http://example.com/my/route/123'); + expect(sentrySpan?.data).toEqual({ + 'http.method': 'GET', + 'http.target': '/my/route/123', + 'http.url': 'http://example.com/my/route/123?what=123#myHash', + 'otel.kind': 'INTERNAL', + url: 'http://example.com/my/route/123', + 'http.query': '?what=123', + 'http.fragment': '#myHash', + }); parentOtelSpan.end(); }); diff --git a/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts b/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts new file mode 100644 index 000000000000..b2d1b3654500 --- /dev/null +++ b/packages/opentelemetry-node/test/utils/parseOtelSpanDescription.test.ts @@ -0,0 +1,144 @@ +import { SpanKind } from '@opentelemetry/api'; +import { SemanticAttributes } from '@opentelemetry/semantic-conventions'; + +import { getSanitizedUrl } from '../../src/utils/parseOtelSpanDescription'; + +describe('getSanitizedUrl', () => { + it.each([ + [ + 'works without attributes', + {}, + SpanKind.CLIENT, + { + urlPath: undefined, + url: undefined, + fragment: undefined, + query: undefined, + }, + ], + [ + 'uses url without query for client request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.CLIENT, + { + urlPath: 'http://example.com/', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + [ + 'uses url without hash for client request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/sub#hash', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/sub#hash', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.CLIENT, + { + urlPath: 'http://example.com/sub', + url: 'http://example.com/sub', + fragment: '#hash', + query: undefined, + }, + ], + [ + 'uses route if available for client request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_ROUTE]: '/my-route', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.CLIENT, + { + urlPath: '/my-route', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + [ + 'falls back to target for client request if url not available', + { + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.CLIENT, + { + urlPath: '/', + url: undefined, + fragment: undefined, + query: undefined, + }, + ], + [ + 'uses target without query for server request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.SERVER, + { + urlPath: '/', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + [ + 'uses target without hash for server request', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/sub#hash', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.SERVER, + { + urlPath: '/sub', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + [ + 'uses route for server request if available', + { + [SemanticAttributes.HTTP_URL]: 'http://example.com/?what=true', + [SemanticAttributes.HTTP_METHOD]: 'GET', + [SemanticAttributes.HTTP_TARGET]: '/?what=true', + [SemanticAttributes.HTTP_ROUTE]: '/my-route', + [SemanticAttributes.HTTP_HOST]: 'example.com:80', + [SemanticAttributes.HTTP_STATUS_CODE]: 200, + }, + SpanKind.SERVER, + { + urlPath: '/my-route', + url: 'http://example.com/', + fragment: undefined, + query: '?what=true', + }, + ], + ])('%s', (_, attributes, kind, expected) => { + const actual = getSanitizedUrl(attributes, kind); + + expect(actual).toEqual(expected); + }); +}); From 0ca73897a895d1673d0a7b2c3145b10dd52953f3 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 17 Jul 2023 18:10:10 +0200 Subject: [PATCH 08/13] fix(nextjs): Avoid importing `SentryWebpackPlugin` in dev mode (#8557) As reported in #8541, our NextJS SDK currently breaks dev mode for the newest NextJS 13.4.10 version I still have absolutely no idea which of the changes in [13.4.10](https://github.com/vercel/next.js/releases/tag/v13.4.10) is causing this. However, I traced the error back and it happens as soon as our NextJS SDK package requires @sentry/webpack-plugin: * @sentry/nextjs calls `require('@sentry/webpack-plugin')` * @sentry/webpack-plugin calls `const { RawSource } = require('webpack-sources');` * For _whatever_ reason, NextJS can't require `webpack-sources` and throws Since we don't enable our Webpack plugin [in dev mode](https://github.com/getsentry/sentry-javascript/blob/723f851f358b75cd39da353804c51ff27ebb0c11/packages/nextjs/src/config/webpack.ts#L305) anyway, one way to get rid of this error is to only require it if we're _not_ in dev mode. This hotfix therefore moves the top-level require of `@sentry/webpack-plugin` to a dynamic require. This isn't a great solution and honestly quite ugly but if it unblocks users for now I'd say we merge it. I think we should definitely revisit this though once we know more about why NextJS suddenly isn't able to import `webpack-sources`. closes #8541 --- packages/nextjs/src/config/webpack.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/config/webpack.ts b/packages/nextjs/src/config/webpack.ts index fd5b36f9a789..a923c57cfef1 100644 --- a/packages/nextjs/src/config/webpack.ts +++ b/packages/nextjs/src/config/webpack.ts @@ -1,8 +1,7 @@ /* eslint-disable complexity */ /* eslint-disable max-lines */ import { getSentryRelease } from '@sentry/node'; -import { arrayify, dropUndefinedKeys, escapeStringForRegex, logger } from '@sentry/utils'; -import { default as SentryWebpackPlugin } from '@sentry/webpack-plugin'; +import { arrayify, dropUndefinedKeys, escapeStringForRegex, loadModule, logger } from '@sentry/utils'; import * as chalk from 'chalk'; import * as fs from 'fs'; import * as path from 'path'; @@ -313,8 +312,11 @@ export function constructWebpackConfigFunction( // without, the option to use `hidden-source-map` only applies to the client-side build. newConfig.devtool = userSentryOptions.hideSourceMaps && !isServer ? 'hidden-source-map' : 'source-map'; + const SentryWebpackPlugin = loadModule('@sentry/webpack-plugin'); + newConfig.plugins = newConfig.plugins || []; newConfig.plugins.push( + // @ts-expect-error - this exists, the dynamic import just doesn't know about it new SentryWebpackPlugin( getWebpackPluginOptions(buildContext, userSentryWebpackPluginOptions, userSentryOptions), ), @@ -767,6 +769,9 @@ function shouldEnableWebpackPlugin(buildContext: BuildContext, userSentryOptions // architecture-specific version of the `sentry-cli` binary. If `yarn install`, `npm install`, or `npm ci` are run // with the `--ignore-scripts` option, this will be blocked and the missing binary will cause an error when users // try to build their apps. + const SentryWebpackPlugin = loadModule('@sentry/webpack-plugin'); + + // @ts-expect-error - this exists, the dynamic import just doesn't know it if (!SentryWebpackPlugin.cliBinaryExists()) { // eslint-disable-next-line no-console console.error( From e6cc1243cf386b3392c45e88a4a8939e42370ee6 Mon Sep 17 00:00:00 2001 From: Kev <6111995+k-fish@users.noreply.github.com> Date: Mon, 17 Jul 2023 12:55:20 -0400 Subject: [PATCH 09/13] feat(tracing): Add more network timings to http calls (#8540) --- .../tracing/browsertracing/http-timings/test.ts | 7 +++++++ .../tracing-internal/src/browser/request.ts | 17 ++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/packages/browser-integration-tests/suites/tracing/browsertracing/http-timings/test.ts b/packages/browser-integration-tests/suites/tracing/browsertracing/http-timings/test.ts index c4b5d3e92e62..566c14297897 100644 --- a/packages/browser-integration-tests/suites/tracing/browsertracing/http-timings/test.ts +++ b/packages/browser-integration-tests/suites/tracing/browsertracing/http-timings/test.ts @@ -40,9 +40,16 @@ sentryTest('should create fetch spans with http timing', async ({ browserName, g timestamp: expect.any(Number), trace_id: tracingEvent.contexts?.trace?.trace_id, data: expect.objectContaining({ + 'http.request.redirect_start': expect.any(Number), + 'http.request.fetch_start': expect.any(Number), + 'http.request.domain_lookup_start': expect.any(Number), + 'http.request.domain_lookup_end': expect.any(Number), 'http.request.connect_start': expect.any(Number), + 'http.request.secure_connection_start': expect.any(Number), + 'http.request.connection_end': expect.any(Number), 'http.request.request_start': expect.any(Number), 'http.request.response_start': expect.any(Number), + 'http.request.response_end': expect.any(Number), 'network.protocol.version': expect.any(String), }), }), diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 071b2146bb16..3407c33a8f2a 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -182,6 +182,10 @@ function addHTTPTimings(span: Span): void { }); } +function getAbsoluteTime(time: number): number { + return ((browserPerformanceTimeOrigin || performance.timeOrigin) + time) / 1000; +} + function resourceTimingEntryToSpanData(resourceTiming: PerformanceResourceTiming): [string, string | number][] { const version = resourceTiming.nextHopProtocol.split('/')[1] || 'none'; @@ -195,9 +199,16 @@ function resourceTimingEntryToSpanData(resourceTiming: PerformanceResourceTiming } return [ ...timingSpanData, - ['http.request.connect_start', (browserPerformanceTimeOrigin + resourceTiming.connectStart) / 1000], - ['http.request.request_start', (browserPerformanceTimeOrigin + resourceTiming.requestStart) / 1000], - ['http.request.response_start', (browserPerformanceTimeOrigin + resourceTiming.responseStart) / 1000], + ['http.request.redirect_start', getAbsoluteTime(resourceTiming.redirectStart)], + ['http.request.fetch_start', getAbsoluteTime(resourceTiming.fetchStart)], + ['http.request.domain_lookup_start', getAbsoluteTime(resourceTiming.domainLookupStart)], + ['http.request.domain_lookup_end', getAbsoluteTime(resourceTiming.domainLookupEnd)], + ['http.request.connect_start', getAbsoluteTime(resourceTiming.connectStart)], + ['http.request.secure_connection_start', getAbsoluteTime(resourceTiming.secureConnectionStart)], + ['http.request.connection_end', getAbsoluteTime(resourceTiming.connectEnd)], + ['http.request.request_start', getAbsoluteTime(resourceTiming.requestStart)], + ['http.request.response_start', getAbsoluteTime(resourceTiming.responseStart)], + ['http.request.response_end', getAbsoluteTime(resourceTiming.responseEnd)], ]; } From 6b009c00be36f9fdf75ec9a98396da8ea2906495 Mon Sep 17 00:00:00 2001 From: Kev <6111995+k-fish@users.noreply.github.com> Date: Mon, 17 Jul 2023 13:32:40 -0400 Subject: [PATCH 10/13] fix(tracing): Improve network.protocol.version (#8502) Protocols are from https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids which don't exactly match the OSI. We can consider adding a lookup table later if people are finding this insufficient. Co-authored-by: Abhijeet Prasad --- .../integration/test/client/pageload.test.ts | 7 ++- .../tracing-internal/src/browser/request.ts | 38 ++++++++++-- .../test/browser/request.test.ts | 59 ++++++++++++++++++- 3 files changed, 98 insertions(+), 6 deletions(-) diff --git a/packages/remix/test/integration/test/client/pageload.test.ts b/packages/remix/test/integration/test/client/pageload.test.ts index 7c49e4ac9c8c..59a8e331668e 100644 --- a/packages/remix/test/integration/test/client/pageload.test.ts +++ b/packages/remix/test/integration/test/client/pageload.test.ts @@ -4,7 +4,12 @@ import { getFirstSentryEnvelopeRequest } from './utils/helpers'; import { test, expect } from '@playwright/test'; import { Event } from '@sentry/types'; -test('should add `pageload` transaction on load.', async ({ page }) => { +test('should add `pageload` transaction on load.', async ({ page, browserName }) => { + // This test is flaky on firefox + if (browserName === 'firefox') { + test.skip(); + } + const envelope = await getFirstSentryEnvelopeRequest(page, '/'); expect(envelope.contexts?.trace.op).toBe('pageload'); diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index 3407c33a8f2a..a8e3034d597e 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -182,17 +182,47 @@ function addHTTPTimings(span: Span): void { }); } +/** + * Converts ALPN protocol ids to name and version. + * + * (https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids) + * @param nextHopProtocol PerformanceResourceTiming.nextHopProtocol + */ +export function extractNetworkProtocol(nextHopProtocol: string): { name: string; version: string } { + let name = 'unknown'; + let version = 'unknown'; + let _name = ''; + for (const char of nextHopProtocol) { + // http/1.1 etc. + if (char === '/') { + [name, version] = nextHopProtocol.split('/'); + break; + } + // h2, h3 etc. + if (!isNaN(Number(char))) { + name = _name === 'h' ? 'http' : _name; + version = nextHopProtocol.split(_name)[1]; + break; + } + _name += char; + } + if (_name === nextHopProtocol) { + // webrtc, ftp, etc. + name = _name; + } + return { name, version }; +} + function getAbsoluteTime(time: number): number { return ((browserPerformanceTimeOrigin || performance.timeOrigin) + time) / 1000; } function resourceTimingEntryToSpanData(resourceTiming: PerformanceResourceTiming): [string, string | number][] { - const version = resourceTiming.nextHopProtocol.split('/')[1] || 'none'; + const { name, version } = extractNetworkProtocol(resourceTiming.nextHopProtocol); const timingSpanData: [string, string | number][] = []; - if (version) { - timingSpanData.push(['network.protocol.version', version]); - } + + timingSpanData.push(['network.protocol.version', version], ['network.protocol.name', name]); if (!browserPerformanceTimeOrigin) { return timingSpanData; diff --git a/packages/tracing-internal/test/browser/request.test.ts b/packages/tracing-internal/test/browser/request.test.ts index 9c8307e97fd7..992b50768428 100644 --- a/packages/tracing-internal/test/browser/request.test.ts +++ b/packages/tracing-internal/test/browser/request.test.ts @@ -7,7 +7,13 @@ import type { Transaction } from '../../../tracing/src'; import { addExtensionMethods, Span, spanStatusfromHttpCode } from '../../../tracing/src'; import { getDefaultBrowserClientOptions } from '../../../tracing/test/testutils'; import type { FetchData, XHRData } from '../../src/browser/request'; -import { fetchCallback, instrumentOutgoingRequests, shouldAttachHeaders, xhrCallback } from '../../src/browser/request'; +import { + extractNetworkProtocol, + fetchCallback, + instrumentOutgoingRequests, + shouldAttachHeaders, + xhrCallback, +} from '../../src/browser/request'; import { TestClient } from '../utils/TestClient'; beforeAll(() => { @@ -388,6 +394,57 @@ describe('callbacks', () => { }); }); +interface ProtocolInfo { + name: string; + version: string; +} + +describe('HTTPTimings', () => { + describe('Extracting version from ALPN protocol', () => { + const nextHopToNetworkVersion: Record = { + 'http/0.9': { name: 'http', version: '0.9' }, + 'http/1.0': { name: 'http', version: '1.0' }, + 'http/1.1': { name: 'http', version: '1.1' }, + 'spdy/1': { name: 'spdy', version: '1' }, + 'spdy/2': { name: 'spdy', version: '2' }, + 'spdy/3': { name: 'spdy', version: '3' }, + 'stun.turn': { name: 'stun.turn', version: 'unknown' }, + 'stun.nat-discovery': { name: 'stun.nat-discovery', version: 'unknown' }, + h2: { name: 'http', version: '2' }, + h2c: { name: 'http', version: '2c' }, + webrtc: { name: 'webrtc', version: 'unknown' }, + 'c-webrtc': { name: 'c-webrtc', version: 'unknown' }, + ftp: { name: 'ftp', version: 'unknown' }, + imap: { name: 'imap', version: 'unknown' }, + pop3: { name: 'pop', version: '3' }, + managesieve: { name: 'managesieve', version: 'unknown' }, + coap: { name: 'coap', version: 'unknown' }, + 'xmpp-client': { name: 'xmpp-client', version: 'unknown' }, + 'xmpp-server': { name: 'xmpp-server', version: 'unknown' }, + 'acme-tls/1': { name: 'acme-tls', version: '1' }, + mqtt: { name: 'mqtt', version: 'unknown' }, + dot: { name: 'dot', version: 'unknown' }, + 'ntske/1': { name: 'ntske', version: '1' }, + sunrpc: { name: 'sunrpc', version: 'unknown' }, + h3: { name: 'http', version: '3' }, + smb: { name: 'smb', version: 'unknown' }, + irc: { name: 'irc', version: 'unknown' }, + nntp: { name: 'nntp', version: 'unknown' }, + nnsp: { name: 'nnsp', version: 'unknown' }, + doq: { name: 'doq', version: 'unknown' }, + 'sip/2': { name: 'sip', version: '2' }, + 'tds/8.0': { name: 'tds', version: '8.0' }, + dicom: { name: 'dicom', version: 'unknown' }, + }; + + const protocols = Object.keys(nextHopToNetworkVersion); + for (const protocol of protocols) { + const expected: ProtocolInfo = nextHopToNetworkVersion[protocol]; + expect(extractNetworkProtocol(protocol)).toMatchObject(expected); + } + }); +}); + describe('shouldAttachHeaders', () => { describe('should prefer `tracePropagationTargets` over defaults', () => { it('should return `true` if the url matches the new tracePropagationTargets', () => { From c2bd0914a4ade946b100263e9e4eeff9164984c5 Mon Sep 17 00:00:00 2001 From: Tim Fish Date: Mon, 17 Jul 2023 18:35:45 +0100 Subject: [PATCH 11/13] feat(core): Allow multiplexed transport to send to multiple releases (#8559) The multiplexed transport can already route events to different or multiple DSNs but we also need to be able to route to specific releases too. In a page with micro-frontends, it's possible (and probably even quite common) to be using the same dependency multiple times but different versions (ie. different releases). Depending on where an error occurs we might want to send an event to `cool-internal-components@1.0.0-beta` or `cool-internal-components@0.9.0` at the same DSN. This PR: - Adds a private `makeOverrideReleaseTransport` which can used to wrap a transport and override the release on all events - Modifies `makeMultiplexedTransport` so it now creates a transport for each unique dsn/release pair - And uses `makeOverrideReleaseTransport` whenever a release is returned from the callback --- packages/core/src/transports/multiplexed.ts | 57 ++++++++++++++++--- .../test/lib/transports/multiplexed.test.ts | 27 +++++++-- 2 files changed, 73 insertions(+), 11 deletions(-) diff --git a/packages/core/src/transports/multiplexed.ts b/packages/core/src/transports/multiplexed.ts index c7045f4b2686..a496a8adcd6f 100644 --- a/packages/core/src/transports/multiplexed.ts +++ b/packages/core/src/transports/multiplexed.ts @@ -24,9 +24,15 @@ interface MatchParam { getEvent(types?: EnvelopeItemType[]): Event | undefined; } -type Matcher = (param: MatchParam) => string[]; +type RouteTo = { dsn: string; release: string }; +type Matcher = (param: MatchParam) => (string | RouteTo)[]; -function eventFromEnvelope(env: Envelope, types: EnvelopeItemType[]): Event | undefined { +/** + * Gets an event from an envelope. + * + * This is only exported for use in the tests + */ +export function eventFromEnvelope(env: Envelope, types: EnvelopeItemType[]): Event | undefined { let event: Event | undefined; forEachEnvelopeItem(env, (item, type) => { @@ -40,6 +46,30 @@ function eventFromEnvelope(env: Envelope, types: EnvelopeItemType[]): Event | un return event; } +/** + * Creates a transport that overrides the release on all events. + */ +function makeOverrideReleaseTransport( + createTransport: (options: TO) => Transport, + release: string, +): (options: TO) => Transport { + return options => { + const transport = createTransport(options); + + return { + send: async (envelope: Envelope): Promise => { + const event = eventFromEnvelope(envelope, ['event', 'transaction', 'profile', 'replay_event']); + + if (event) { + event.release = release; + } + return transport.send(envelope); + }, + flush: timeout => transport.flush(timeout), + }; + }; +} + /** * Creates a transport that can send events to different DSNs depending on the envelope contents. */ @@ -51,17 +81,24 @@ export function makeMultiplexedTransport( const fallbackTransport = createTransport(options); const otherTransports: Record = {}; - function getTransport(dsn: string): Transport | undefined { - if (!otherTransports[dsn]) { + function getTransport(dsn: string, release: string | undefined): Transport | undefined { + // We create a transport for every unique dsn/release combination as there may be code from multiple releases in + // use at the same time + const key = release ? `${dsn}:${release}` : dsn; + + if (!otherTransports[key]) { const validatedDsn = dsnFromString(dsn); if (!validatedDsn) { return undefined; } const url = getEnvelopeEndpointWithUrlEncodedAuth(validatedDsn); - otherTransports[dsn] = createTransport({ ...options, url }); + + otherTransports[key] = release + ? makeOverrideReleaseTransport(createTransport, release)({ ...options, url }) + : createTransport({ ...options, url }); } - return otherTransports[dsn]; + return otherTransports[key]; } async function send(envelope: Envelope): Promise { @@ -71,7 +108,13 @@ export function makeMultiplexedTransport( } const transports = matcher({ envelope, getEvent }) - .map(dsn => getTransport(dsn)) + .map(result => { + if (typeof result === 'string') { + return getTransport(result, undefined); + } else { + return getTransport(result.dsn, result.release); + } + }) .filter((t): t is Transport => !!t); // If we have no transports to send to, use the fallback transport diff --git a/packages/core/test/lib/transports/multiplexed.test.ts b/packages/core/test/lib/transports/multiplexed.test.ts index 2d2dcb5ce46d..446d185f9301 100644 --- a/packages/core/test/lib/transports/multiplexed.test.ts +++ b/packages/core/test/lib/transports/multiplexed.test.ts @@ -6,10 +6,11 @@ import type { TransactionEvent, Transport, } from '@sentry/types'; -import { createClientReportEnvelope, createEnvelope, dsnFromString } from '@sentry/utils'; -import { TextEncoder } from 'util'; +import { createClientReportEnvelope, createEnvelope, dsnFromString, parseEnvelope } from '@sentry/utils'; +import { TextDecoder, TextEncoder } from 'util'; import { createTransport, getEnvelopeEndpointWithUrlEncodedAuth, makeMultiplexedTransport } from '../../../src'; +import { eventFromEnvelope } from '../../../src/transports/multiplexed'; const DSN1 = 'https://1234@5678.ingest.sentry.io/4321'; const DSN1_URL = getEnvelopeEndpointWithUrlEncodedAuth(dsnFromString(DSN1)!); @@ -47,7 +48,7 @@ const CLIENT_REPORT_ENVELOPE = createClientReportEnvelope( 123456, ); -type Assertion = (url: string, body: string | Uint8Array) => void; +type Assertion = (url: string, release: string | undefined, body: string | Uint8Array) => void; const createTestTransport = (...assertions: Assertion[]): ((options: BaseTransportOptions) => Transport) => { return (options: BaseTransportOptions) => @@ -57,7 +58,10 @@ const createTestTransport = (...assertions: Assertion[]): ((options: BaseTranspo if (!assertion) { throw new Error('No assertion left'); } - assertion(options.url, request.body); + + const event = eventFromEnvelope(parseEnvelope(request.body, new TextEncoder(), new TextDecoder()), ['event']); + + assertion(options.url, event?.release, request.body); resolve({ statusCode: 200 }); }); }); @@ -111,6 +115,21 @@ describe('makeMultiplexedTransport', () => { await transport.send(ERROR_ENVELOPE); }); + it('DSN and release can be overridden via match callback', async () => { + expect.assertions(2); + + const makeTransport = makeMultiplexedTransport( + createTestTransport((url, release) => { + expect(url).toBe(DSN2_URL); + expect(release).toBe('something@1.0.0'); + }), + () => [{ dsn: DSN2, release: 'something@1.0.0' }], + ); + + const transport = makeTransport({ url: DSN1_URL, ...transportOptions }); + await transport.send(ERROR_ENVELOPE); + }); + it('match callback can return multiple DSNs', async () => { expect.assertions(2); From 564af01aeb60706dce0694c662df8a34932c4176 Mon Sep 17 00:00:00 2001 From: Kev <6111995+k-fish@users.noreply.github.com> Date: Mon, 17 Jul 2023 14:15:09 -0400 Subject: [PATCH 12/13] feat(tracing): Bring http timings out of experiment (#8563) Co-authored-by: Abhijeet Prasad --- .../src/browser/browsertracing.ts | 7 ++--- .../tracing-internal/src/browser/request.ts | 31 ++++++++++++------- .../test/browser/browsertracing.test.ts | 6 ++-- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/packages/tracing-internal/src/browser/browsertracing.ts b/packages/tracing-internal/src/browser/browsertracing.ts index 4be633821a1a..13cde107074f 100644 --- a/packages/tracing-internal/src/browser/browsertracing.ts +++ b/packages/tracing-internal/src/browser/browsertracing.ts @@ -105,7 +105,6 @@ export interface BrowserTracingOptions extends RequestInstrumentationOptions { _experiments: Partial<{ enableLongTask: boolean; enableInteractions: boolean; - enableHTTPTimings: boolean; onStartRouteTransaction: (t: Transaction | undefined, ctx: TransactionContext, getCurrentHub: () => Hub) => void; }>; @@ -140,6 +139,7 @@ const DEFAULT_BROWSER_TRACING_OPTIONS: BrowserTracingOptions = { startTransactionOnLocationChange: true, startTransactionOnPageLoad: true, enableLongTask: true, + _experiments: {}, ...defaultRequestInstrumentationOptions, }; @@ -230,6 +230,7 @@ export class BrowserTracing implements Integration { traceFetch, traceXHR, shouldCreateSpanForRequest, + enableHTTPTimings, _experiments, } = this.options; @@ -277,9 +278,7 @@ export class BrowserTracing implements Integration { traceXHR, tracePropagationTargets, shouldCreateSpanForRequest, - _experiments: { - enableHTTPTimings: _experiments.enableHTTPTimings, - }, + enableHTTPTimings, }); } diff --git a/packages/tracing-internal/src/browser/request.ts b/packages/tracing-internal/src/browser/request.ts index a8e3034d597e..2062ab5ce587 100644 --- a/packages/tracing-internal/src/browser/request.ts +++ b/packages/tracing-internal/src/browser/request.ts @@ -16,13 +16,6 @@ export const DEFAULT_TRACE_PROPAGATION_TARGETS = ['localhost', /^\/(?!\/)/]; /** Options for Request Instrumentation */ export interface RequestInstrumentationOptions { - /** - * Allow experiments for the request instrumentation. - */ - _experiments: Partial<{ - enableHTTPTimings: boolean; - }>; - /** * @deprecated Will be removed in v8. * Use `shouldCreateSpanForRequest` to control span creation and `tracePropagationTargets` to control @@ -52,6 +45,13 @@ export interface RequestInstrumentationOptions { */ traceXHR: boolean; + /** + * If true, Sentry will capture http timings and add them to the corresponding http spans. + * + * Default: true + */ + enableHTTPTimings: boolean; + /** * This function will be called before creating a span for a request with the given url. * Return false if you don't want a span for the given url. @@ -114,16 +114,23 @@ type PolymorphicRequestHeaders = export const defaultRequestInstrumentationOptions: RequestInstrumentationOptions = { traceFetch: true, traceXHR: true, + enableHTTPTimings: true, // TODO (v8): Remove this property tracingOrigins: DEFAULT_TRACE_PROPAGATION_TARGETS, tracePropagationTargets: DEFAULT_TRACE_PROPAGATION_TARGETS, - _experiments: {}, }; /** Registers span creators for xhr and fetch requests */ export function instrumentOutgoingRequests(_options?: Partial): void { - // eslint-disable-next-line deprecation/deprecation - const { traceFetch, traceXHR, tracePropagationTargets, tracingOrigins, shouldCreateSpanForRequest, _experiments } = { + const { + traceFetch, + traceXHR, + tracePropagationTargets, + // eslint-disable-next-line deprecation/deprecation + tracingOrigins, + shouldCreateSpanForRequest, + enableHTTPTimings, + } = { traceFetch: defaultRequestInstrumentationOptions.traceFetch, traceXHR: defaultRequestInstrumentationOptions.traceXHR, ..._options, @@ -143,7 +150,7 @@ export function instrumentOutgoingRequests(_options?: Partial { const createdSpan = fetchCallback(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); - if (_experiments?.enableHTTPTimings && createdSpan) { + if (enableHTTPTimings && createdSpan) { addHTTPTimings(createdSpan); } }); @@ -152,7 +159,7 @@ export function instrumentOutgoingRequests(_options?: Partial { const createdSpan = xhrCallback(handlerData, shouldCreateSpan, shouldAttachHeadersWithTargets, spans); - if (_experiments?.enableHTTPTimings && createdSpan) { + if (enableHTTPTimings && createdSpan) { addHTTPTimings(createdSpan); } }); diff --git a/packages/tracing-internal/test/browser/browsertracing.test.ts b/packages/tracing-internal/test/browser/browsertracing.test.ts index 0754afd65fc8..e6a9eff3fb82 100644 --- a/packages/tracing-internal/test/browser/browsertracing.test.ts +++ b/packages/tracing-internal/test/browser/browsertracing.test.ts @@ -95,6 +95,7 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { expect(browserTracing.options).toEqual({ enableLongTask: true, + _experiments: {}, ...TRACING_DEFAULTS, markBackgroundTransactions: true, routingInstrumentation: instrumentRoutingWithDefaults, @@ -132,6 +133,7 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { expect(browserTracing.options).toEqual({ enableLongTask: false, + _experiments: {}, ...TRACING_DEFAULTS, markBackgroundTransactions: true, routingInstrumentation: instrumentRoutingWithDefaults, @@ -246,7 +248,7 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { traceFetch: true, traceXHR: true, tracePropagationTargets: ['something'], - _experiments: {}, + enableHTTPTimings: true, }); }); @@ -260,7 +262,7 @@ conditionalTest({ min: 10 })('BrowserTracing', () => { }); expect(instrumentOutgoingRequestsMock).toHaveBeenCalledWith({ - _experiments: {}, + enableHTTPTimings: true, traceFetch: true, traceXHR: true, tracePropagationTargets: ['something-else'], From 5eb0bdd61cd665cd921abb78bc6fe6ab2fb823fb Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 17 Jul 2023 18:16:28 +0200 Subject: [PATCH 13/13] meta: Update CHANGELOG for 7.59.0 --- CHANGELOG.md | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b7a9fe0e146..5db4ecfabf93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,60 @@ - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott +## 7.59.0 + +### Important Changes + +- **- feat(remix): Add Remix v2 support (#8415)** + +This release adds support for Remix v2 future flags, in particular for new error handling utilities of Remix v2. We heavily recommend you switch to using `v2_errorBoundary` future flag to get the best error handling experience with Sentry. + +To capture errors from [v2 client-side ErrorBoundary](https://remix.run/docs/en/main/route/error-boundary-v2), you should define your own `ErrorBoundary` in `root.tsx` and use `Sentry.captureRemixErrorBoundaryError` helper to capture the error. + +```typescript +// root.tsx +import { captureRemixErrorBoundaryError } from "@sentry/remix"; + +export const ErrorBoundary: V2_ErrorBoundaryComponent = () => { + const error = useRouteError(); + + captureRemixErrorBoundaryError(error); + + return
...
; +}; +``` + +For server-side errors, define a [`handleError`](https://remix.run/docs/en/main/file-conventions/entry.server#handleerror) function in your server entry point and use the `Sentry.captureRemixServerException` helper to capture the error. + +```ts +// entry.server.tsx +export function handleError( + error: unknown, + { request }: DataFunctionArgs +): void { + if (error instanceof Error) { + Sentry.captureRemixServerException(error, "remix.server", request); + } else { + // Optionally capture non-Error objects + Sentry.captureException(error); + } +} +``` + +For more details, see the Sentry [Remix SDK](https://docs.sentry.io/platforms/javascript/guides/remix/) documentation. + +### Other Changes + +- feat(core): Add `ModuleMetadata` integration (#8475) +- feat(core): Allow multiplexed transport to send to multiple releases (#8559) +- feat(tracing): Add more network timings to http calls (#8540) +- feat(tracing): Bring http timings out of experiment (#8563) +- fix(nextjs): Avoid importing `SentryWebpackPlugin` in dev mode (#8557) +- fix(otel): Use `HTTP_URL` attribute for client requests (#8539) +- fix(replay): Better session storage check (#8547) +- fix(replay): Handle errors in `beforeAddRecordingEvent` callback (#8548) +- fix(tracing): Improve network.protocol.version (#8502) + ## 7.58.1 - fix(node): Set propagation context even when tracingOptions are not defined (#8517)