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.

Sign up for GitHub

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

Jump to bottom

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
Open

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

wants to merge 9 commits into from
+345 −3

Conversation

Zen-cronic
Copy link
Contributor

@Zen-cronic Zen-cronic commented Jul 24, 2024 •
edited
Loading

Fixes #12844

For the tests, would dev-packages/node-integration-tests/suites or dev-packages/e2e-tests/test-applications/node-fastify be a more appropriate ___location?

I also noticed that there's currently no test for fastify in node-integration.

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

mydea
mydea reviewed Jul 25, 2024
View reviewed changes
packages/node/src/integrations/tracing/fastify.ts
@@ -79,7 +78,7 @@ 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.

@Zen-cronic Zen-cronic changed the title fix(node): Remove deprecated routerPath option of fastify request object fix(node): Remove deprecated routerPath warning of fastify request object Jul 25, 2024
@Zen-cronic
Copy link
Contributor Author

Zen-cronic commented Jul 25, 2024 •
edited
Loading

About the test, we can spyOn/mock the process.emitWarning() (which fastify uses), and assert that it's never invoked. But the test runner uses Axios, which throws its own error when faced with a 404. This happens before reaching the fastify code. So mocking would have no effect.

Although the deprecation warning is not logged (when manually tested with the fixed code), I'm stuck at including the case into the test suite. Currently, only the AxiosError is tested. Any thoughts?

@AbhiPrasad
Copy link
Member

I'm stuck at including the case into the test suite. Currently, only the AxiosError is tested

We should probably move away from Axios in general, I think this limitation is fine because we know the implementation works in OTEL.

Let's just leave a comment on the test about the deprecation warning not being logged.

AbhiPrasad
AbhiPrasad approved these changes Jul 31, 2024
View reviewed changes
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

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

After we add a comment to the test, and rebase this PR to latest, it's good to go to merge in my eyes.

Thanks for the PR @Zen-cronic (also nice to see some Canadian contributions 🇨🇦)

@Zen-cronic
Copy link
Contributor Author

Zen-cronic commented Aug 2, 2024 •
edited
Loading

After we add a comment to the test, and rebase this PR to latest, it's good to go to merge in my eyes.

Thanks for the PR @Zen-cronic (also nice to see some Canadian contributions 🇨🇦)

Happy to help :)

Also I believe my rebase works correctly?

Edit: I think I messed up.

@AbhiPrasad AbhiPrasad self-assigned this Aug 2, 2024
@AbhiPrasad
Copy link
Member

assigning myself to help get this merged in!

@AbhiPrasad AbhiPrasad added the Integration: fastify Issues related to Fastify support for the Sentry Node SDK label Aug 2, 2024
AbhiPrasad
AbhiPrasad reviewed Aug 2, 2024
View reviewed changes
dev-packages/node-integration-tests/package.json
@@ -42,6 +42,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 😬

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) August 2, 2024 19:06
@Zen-cronic
Copy link
Contributor Author

Thanks for helping out!

@mydea mydea force-pushed the fix/node-fastify-deprecation branch from 8525aa8 to f131c74 Compare August 28, 2024 06:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mydea mydea mydea left review comments

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

Assignees

@AbhiPrasad AbhiPrasad

Labels
Integration: fastify Issues related to Fastify support for the Sentry Node SDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RouterPath deprecation warning when using Fastify integration on 404
3 participants
@Zen-cronic @AbhiPrasad @mydea