Skip to content

feat(remix): Update remix SDK to be OTEL-powered #11031

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

Merged
merged 1 commit into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -871,7 +871,7 @@ jobs:
yarn test

job_remix_integration_tests:
name: Remix v${{ matrix.remix }} (Node ${{ matrix.node }}) ${{ matrix.tracingIntegration && 'TracingIntegration'}} Tests
name: Remix v${{ matrix.remix }} (Node ${{ matrix.node }}) Tests
needs: [job_get_metadata, job_build]
if: needs.job_get_metadata.outputs.changed_remix == 'true' || github.event_name != 'pull_request'
runs-on: ubuntu-20.04
Expand Down
Original file line number Diff line number Diff line change
@@ -1,25 +1,25 @@
import * as Sentry from '@sentry/remix';

Sentry.init({
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mydea, @AbhiPrasad

We should update the wizard and the docs for this right? Also a breaking change warning for the changelog?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get to the docs/wizard changes afterwards, this is only going to be released with an alpha.

Though we maybe should add a migration note 🤔

tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production!
environment: 'qa', // dynamic sampling bias to keep transactions
dsn: process.env.E2E_TEST_DSN,
tunnel: 'http://localhost:3031/', // proxy server
});

import { PassThrough } from 'node:stream';

import type { AppLoadContext, EntryContext } from '@remix-run/node';
import { createReadableStreamFromReadable } from '@remix-run/node';
import { installGlobals } from '@remix-run/node';
import { RemixServer } from '@remix-run/react';
import * as Sentry from '@sentry/remix';
import * as isbotModule from 'isbot';
import { renderToPipeableStream } from 'react-dom/server';

installGlobals();

const ABORT_DELAY = 5_000;

Sentry.init({
environment: 'qa', // dynamic sampling bias to keep transactions
dsn: process.env.E2E_TEST_DSN,
// Performance Monitoring
tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production!
tunnel: 'http://localhost:3031/', // proxy server
});

export const handleError = Sentry.wrapRemixHandleError;

export default function handleRequest(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page
// We use this to identify the transactions
const testTag = uuid4();

// no server span here!
const httpServerTransactionPromise = waitForTransaction('create-remix-app-express-vite-dev', transactionEvent => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually instrumented now, without us having to do anything - probably the express instrumentation, nice!

return (
transactionEvent.type === 'transaction' &&
transactionEvent.contexts?.trace?.op === 'http.server' &&
transactionEvent.tags?.['sentry_test'] === testTag
);
});

const pageLoadTransactionPromise = waitForTransaction('create-remix-app-express-vite-dev', transactionEvent => {
return (
Expand All @@ -20,16 +26,25 @@ test('Sends two linked transactions (server & client) to Sentry', async ({ page
page.goto(`/?tag=${testTag}`);

const pageloadTransaction = await pageLoadTransactionPromise;
const httpServerTransaction = await httpServerTransactionPromise;

expect(pageloadTransaction).toBeDefined();
expect(httpServerTransaction).toBeDefined();

const httpServerTraceId = httpServerTransaction.contexts?.trace?.trace_id;
const httpServerSpanId = httpServerTransaction.contexts?.trace?.span_id;

const pageLoadTraceId = pageloadTransaction.contexts?.trace?.trace_id;
const pageLoadSpanId = pageloadTransaction.contexts?.trace?.span_id;
const pageLoadParentSpanId = pageloadTransaction.contexts?.trace?.parent_span_id;

expect(httpServerTransaction.transaction).toBe('routes/_index');
expect(pageloadTransaction.transaction).toBe('routes/_index');

expect(pageLoadTraceId).toBeDefined();
expect(pageLoadParentSpanId).toBeUndefined();
expect(pageLoadSpanId).toBeDefined();
expect(httpServerTraceId).toBeDefined();
expect(httpServerSpanId).toBeDefined();

expect(pageLoadTraceId).toEqual(httpServerTraceId);
expect(pageLoadParentSpanId).toEqual(httpServerSpanId);
expect(pageLoadSpanId).not.toEqual(httpServerSpanId);
});
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
import * as Sentry from '@sentry/remix';

Sentry.init({
tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production!
environment: 'qa', // dynamic sampling bias to keep transactions
dsn: process.env.E2E_TEST_DSN,
tunnel: 'http://localhost:3031/', // proxy server
});

/**
* By default, Remix will handle generating the HTTP Response for you.
* You are free to delete this file if you'd like to, but if you ever want it revealed again, you can run `npx remix reveal` ✨
Expand All @@ -10,22 +19,13 @@ import type { AppLoadContext, EntryContext } from '@remix-run/node';
import { createReadableStreamFromReadable } from '@remix-run/node';
import { installGlobals } from '@remix-run/node';
import { RemixServer } from '@remix-run/react';
import * as Sentry from '@sentry/remix';
import isbot from 'isbot';
import { renderToPipeableStream } from 'react-dom/server';

installGlobals();

const ABORT_DELAY = 5_000;

Sentry.init({
environment: 'qa', // dynamic sampling bias to keep transactions
dsn: process.env.E2E_TEST_DSN,
// Performance Monitoring
tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production!
tunnel: 'http://localhost:3031/', // proxy server
});

export const handleError = Sentry.wrapRemixHandleError;

export default function handleRequest(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
import * as Sentry from '@sentry/remix';

Sentry.init({
tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production!
environment: 'qa', // dynamic sampling bias to keep transactions
dsn: process.env.E2E_TEST_DSN,
tunnel: 'http://localhost:3031/', // proxy server
});

/**
* By default, Remix will handle generating the HTTP Response for you.
* You are free to delete this file if you'd like to, but if you ever want it revealed again, you can run `npx remix reveal` ✨
Expand All @@ -9,20 +18,11 @@ import { PassThrough } from 'node:stream';
import type { AppLoadContext, EntryContext } from '@remix-run/node';
import { Response } from '@remix-run/node';
import { RemixServer } from '@remix-run/react';
import * as Sentry from '@sentry/remix';
import isbot from 'isbot';
import { renderToPipeableStream } from 'react-dom/server';

const ABORT_DELAY = 5_000;

Sentry.init({
environment: 'qa', // dynamic sampling bias to keep transactions
dsn: process.env.E2E_TEST_DSN,
// Performance Monitoring
tracesSampleRate: 1.0, // Capture 100% of the transactions, reduce in production!
tunnel: 'http://localhost:3031/', // proxy server
});

export const handleError = Sentry.wrapRemixHandleError;

export default function handleRequest(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ const DEPENDENTS: Dependent[] = [
},
{
package: '@sentry/remix',
compareWith: nodeExperimentalExports,
compareWith: nodeExports,
exports: Object.keys(SentryRemix),
},
{
Expand Down
1 change: 0 additions & 1 deletion packages/astro/src/index.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ export declare function flush(timeout?: number | undefined): PromiseLike<boolean

// eslint-disable-next-line deprecation/deprecation
export declare const makeMain: typeof clientSdk.makeMain;
export declare const getActiveSpan: typeof clientSdk.getActiveSpan;
// eslint-disable-next-line deprecation/deprecation
export declare const getCurrentHub: typeof clientSdk.getCurrentHub;
export declare const getClient: typeof clientSdk.getClient;
Expand Down
2 changes: 2 additions & 0 deletions packages/opentelemetry/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ export {
spanHasStatus,
} from './utils/spanTypes';

export { getDynamicSamplingContextFromSpan } from './utils/dynamicSamplingContext';

export { isSentryRequestSpan } from './utils/isSentryRequest';

export { getActiveSpan } from './utils/getActiveSpan';
Expand Down
2 changes: 2 additions & 0 deletions packages/remix/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@ const baseConfig = require('../../jest/jest.config.js');
module.exports = {
...baseConfig,
testPathIgnorePatterns: ['<rootDir>/build/', '<rootDir>/node_modules/', '<rootDir>/test/integration/'],
// Some tests take longer to finish, as flushing spans with OpenTelemetry takes some more time
testTimeout: 15000,
};
5 changes: 3 additions & 2 deletions packages/remix/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@
"@remix-run/router": "1.x",
"@sentry/cli": "^2.30.0",
"@sentry/core": "8.0.0-alpha.2",
"@sentry/node-experimental": "8.0.0-alpha.2",
"@sentry/node": "8.0.0-alpha.2",
"@sentry/opentelemetry": "8.0.0-alpha.2",
"@sentry/react": "8.0.0-alpha.2",
"@sentry/types": "8.0.0-alpha.2",
"@sentry/utils": "8.0.0-alpha.2",
Expand Down Expand Up @@ -82,7 +83,7 @@
"fix": "eslint . --format stylish --fix",
"lint": "eslint . --format stylish",
"test": "yarn test:unit",
"test:integration": "run-s test:integration:v1 test:integration:v2 test:integration:tracingIntegration",
"test:integration": "run-s test:integration:v1 test:integration:v2",
"test:integration:v1": "run-s test:integration:clean test:integration:prepare test:integration:client test:integration:server",
"test:integration:v2": "export REMIX_VERSION=2 && run-s test:integration:v1",
"test:integration:ci": "run-s test:integration:clean test:integration:prepare test:integration:client:ci test:integration:server",
Expand Down
40 changes: 22 additions & 18 deletions packages/remix/src/index.server.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { applySdkMetadata } from '@sentry/core';
import type { NodeOptions } from '@sentry/node-experimental';
import { getClient, init as nodeInit, setTag } from '@sentry/node-experimental';
import { applySdkMetadata, isInitialized } from '@sentry/core';
import type { NodeOptions } from '@sentry/node';
import { init as nodeInit, setTag } from '@sentry/node';
import { logger } from '@sentry/utils';

import { DEBUG_BUILD } from './utils/debug-build';
Expand All @@ -10,8 +10,6 @@ import type { RemixOptions } from './utils/remixOptions';
// We need to explicitly export @sentry/node as they end up under `default` in ESM builds
// See: https://github.com/getsentry/sentry-javascript/issues/8474
export {
// eslint-disable-next-line deprecation/deprecation
addGlobalEventProcessor,
addEventProcessor,
addBreadcrumb,
addIntegration,
Expand All @@ -22,8 +20,6 @@ export {
captureMessage,
createTransport,
// eslint-disable-next-line deprecation/deprecation
getActiveTransaction,
// eslint-disable-next-line deprecation/deprecation
getCurrentHub,
getClient,
getCurrentScope,
Expand All @@ -46,7 +42,6 @@ export {
setHttpStatus,
withScope,
withIsolationScope,
autoDiscoverNodePerformanceMonitoringIntegrations,
makeNodeTransport,
getDefaultIntegrations,
defaultStackParser,
Expand All @@ -56,7 +51,6 @@ export {
addRequestDataToEvent,
DEFAULT_USER_INCLUDES,
extractRequestData,
Integrations,
consoleIntegration,
onUncaughtExceptionIntegration,
onUnhandledRejectionIntegration,
Expand All @@ -68,7 +62,6 @@ export {
functionToStringIntegration,
inboundFiltersIntegration,
linkedErrorsIntegration,
Handlers,
setMeasurement,
getActiveSpan,
getRootSpan,
Expand All @@ -83,15 +76,30 @@ export {
parameterize,
metrics,
createGetModuleFromFilename,
hapiErrorPlugin,
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
} from '@sentry/node-experimental';
expressIntegration,
expressErrorHandler,
setupExpressErrorHandler,
fastifyIntegration,
graphqlIntegration,
mongoIntegration,
mongooseIntegration,
mysqlIntegration,
mysql2Integration,
nestIntegration,
postgresIntegration,
prismaIntegration,
hapiIntegration,
setupHapiErrorHandler,
spotlightIntegration,
setupFastifyErrorHandler,
} from '@sentry/node';

// Keeping the `*` exports for backwards compatibility and types
export * from '@sentry/node-experimental';
export * from '@sentry/node';

export { captureRemixServerException, wrapRemixHandleError } from './utils/instrumentServer';
export { ErrorBoundary, withErrorBoundary } from '@sentry/react';
Expand All @@ -102,15 +110,11 @@ export { wrapExpressCreateRequestHandler } from './utils/serverAdapters/express'

export type { SentryMetaArgs } from './utils/types';

function sdkAlreadyInitialized(): boolean {
return !!getClient();
}

/** Initializes Sentry Remix SDK on Node. */
export function init(options: RemixOptions): void {
applySdkMetadata(options, 'remix', ['remix', 'node']);

if (sdkAlreadyInitialized()) {
if (isInitialized()) {
DEBUG_BUILD && logger.log('SDK already initialized');

return;
Expand Down
9 changes: 8 additions & 1 deletion packages/remix/src/index.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export declare function init(options: RemixOptions): void;

// We export a merged Integrations object so that users can (at least typing-wise) use all integrations everywhere.
// eslint-disable-next-line deprecation/deprecation
export declare const Integrations: typeof clientSdk.Integrations & typeof serverSdk.Integrations;
export declare const Integrations: typeof clientSdk.Integrations;

export declare const linkedErrorsIntegration: typeof clientSdk.linkedErrorsIntegration;
export declare const contextLinesIntegration: typeof clientSdk.contextLinesIntegration;
Expand All @@ -27,6 +27,13 @@ export declare const defaultStackParser: StackParser;
// methods from `@sentry/core`.
declare const runtime: 'client' | 'server';

// eslint-disable-next-line deprecation/deprecation
export declare const makeMain: typeof clientSdk.makeMain;
// eslint-disable-next-line deprecation/deprecation
export declare const getCurrentHub: typeof clientSdk.getCurrentHub;
export declare const getClient: typeof clientSdk.getClient;
export declare const continueTrace: typeof clientSdk.continueTrace;

export const close = runtime === 'client' ? clientSdk.close : serverSdk.close;
export const flush = runtime === 'client' ? clientSdk.flush : serverSdk.flush;

Expand Down
6 changes: 4 additions & 2 deletions packages/remix/src/utils/instrumentServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ import {
SEMANTIC_ATTRIBUTE_SENTRY_OP,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
captureException,
continueTrace,
getActiveSpan,
getClient,
getDynamicSamplingContextFromSpan,
getRootSpan,
handleCallbackErrors,
hasTracingEnabled,
setHttpStatus,
Expand All @@ -14,7 +16,7 @@ import {
startSpan,
withIsolationScope,
} from '@sentry/core';
import { captureException, getActiveSpan, getRootSpan } from '@sentry/node-experimental';
import { getDynamicSamplingContextFromSpan } from '@sentry/opentelemetry';
import type { Span, TransactionSource, WrappedFunction } from '@sentry/types';
import {
addExceptionMechanism,
Expand Down
2 changes: 1 addition & 1 deletion packages/remix/src/utils/remixOptions.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { NodeOptions } from '@sentry/node-experimental';
import type { NodeOptions } from '@sentry/node';
import type { BrowserOptions } from '@sentry/react';
import type { Options } from '@sentry/types';

Expand Down
2 changes: 1 addition & 1 deletion packages/remix/src/utils/serverAdapters/express.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { getClient, getCurrentHub, hasTracingEnabled, setHttpStatus, withIsolationScope } from '@sentry/core';
import { flush } from '@sentry/node-experimental';
import { flush } from '@sentry/node';
import type { Hub, Span } from '@sentry/types';
import { extractRequestData, fill, isString, logger } from '@sentry/utils';

Expand Down
9 changes: 2 additions & 7 deletions packages/remix/test/index.server.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as SentryNode from '@sentry/node-experimental';
import * as SentryNode from '@sentry/node';

import { Integrations, init } from '../src/index.server';
import { init } from '../src/index.server';

const nodeInit = jest.spyOn(SentryNode, 'init');

Expand Down Expand Up @@ -55,9 +55,4 @@ describe('Server init()', () => {

expect(SentryNode.getIsolationScope().getScopeData().tags).toEqual({ runtime: 'node' });
});

it('has both node and tracing integrations', () => {
expect(Integrations.Apollo).not.toBeUndefined();
expect(Integrations.Http).not.toBeUndefined();
});
});
Loading