Skip to content
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

fix(node): Remove deprecated routerPath warning of fastify request object #13043

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
1 change: 1 addition & 0 deletions dev-packages/node-integration-tests/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"cors": "^2.8.5",
"cron": "^3.1.6",
"express": "^4.17.3",
"fastify": "~4.23.2",
Copy link
Member

Choose a reason for hiding this comment

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

For future readers - we had to pin this at ~4.23.2 because 4.33+ only works with TS 5, which breaks the rest of our tests 😬

"graphql": "^16.3.0",
"http-terminator": "^3.2.0",
"ioredis": "^5.4.1",
Expand Down
20 changes: 20 additions & 0 deletions dev-packages/node-integration-tests/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { AddressInfo } from 'net';
import type { BaseTransportOptions, Envelope, Transport, TransportMakeRequestResponse } from '@sentry/types';
import type { Express } from 'express';
import type { FastifyInstance } from 'fastify';

/**
* Debug logging transport
Expand Down Expand Up @@ -34,6 +35,25 @@ export function startExpressServerAndSendPortToRunner(app: Express, port: number
});
}

/**
* Starts a fastify server and sends the port to the runner
n("sendPortToRunner called");
* @param app Fastify app
* @param port Port to start the app on.
*/
export function startFastifyServerAndSendPortToRunner(
app: FastifyInstance,
port: number | undefined = undefined,
): void {
app.listen({ port: port || 0 }, (_err, address) => {
// Fastify's address (string): http://[::1]:59752, etc.
const addressPort = address.slice(address.lastIndexOf(':') + 1);

// eslint-disable-next-line no-console
console.log(`{"port":${port || addressPort}}`);
});
}

/**
* Sends the port to the runner
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { loggingTransport } from '@sentry-internal/node-integration-tests';
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
release: '1.0',
transport: loggingTransport,
});

import { startFastifyServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests';
import fastify from 'fastify';

const app = fastify();

app.get('/test/deprecated', (_req, res) => {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
res.send({});
});

Sentry.setupFastifyErrorHandler(app);

startFastifyServerAndSendPortToRunner(app);
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import { AxiosError } from 'axios';
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

afterAll(() => {
cleanupChildProcesses();
});

/**
* AxiosError throws before the deprecation warning code is invoked,
* so mocking it would have no effect.
*
* But the deprecation warning is successfully suppressed.
* This can be verified by running the example code as in the original
* issue (https://github.com/getsentry/sentry-javascript/issues/12844)
*/
test('suppress fastify deprecation warning when `routerPath` property is accessed', async () => {
// ensures that the assertions in the catch block are called
expect.assertions(3);

const runner = createRunner(__dirname, 'server.ts').start();

try {
// Axios from `makeRequest` will throw 404.
await runner.makeRequest('get', '/test/deprecated/does-not-exist');
} catch (error) {
expect(error).toBeInstanceOf(AxiosError);
if (error instanceof AxiosError) {
expect(error.message).toBe('Request failed with status code 404');
expect(error.response?.status).toBe(404);
}
}
});
6 changes: 5 additions & 1 deletion packages/node/src/integrations/tracing/fastify.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ interface FastifyRequestRouteInfo {
url?: string;
method?: string;
};
// will deprecate in Fastify 5
routerPath?: string;
}

Expand Down Expand Up @@ -79,7 +80,10 @@ export function setupFastifyErrorHandler(fastify: Fastify): void {

// Taken from Otel Fastify instrumentation:
// https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-fastify/src/instrumentation.ts#L94-L96
const routeName = reqWithRouteInfo.routeOptions?.url || reqWithRouteInfo.routerPath;
Copy link
Member

Choose a reason for hiding this comment

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

Hey, thanks for the PR! Could we do the same change as they did in OTEL:

const routeName = reqWithRouteInfo.routeOptions ? reqWithRouteInfo.routeOptions.url : reqWithRouteInfo.routerPath;

Otherwise, this stops working on older fastify versions, I guess, which we do not want!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh i see! So the routerPath is kept for backwards compatibility.

const routeName = reqWithRouteInfo.routeOptions
? reqWithRouteInfo.routeOptions.url
: reqWithRouteInfo.routerPath;

const method = reqWithRouteInfo.routeOptions?.method || 'GET';

getIsolationScope().setTransactionName(`${method} ${routeName}`);
Expand Down
Loading
Loading