Skip to content

feat(node): Migrate to @fastify/otel #15542

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

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Feb 28, 2025

Resolves: #15130
Depends on: fastify/otel#25 for proper NestJS support.

Opened an issue regarding the problem with fastify types: fastify/otel#32

The behaviour is not exactly the same as before, mainly because the new instrumentation doesn't have a requestHook.

@onurtemizkan onurtemizkan force-pushed the onur/fastify-otel-migration branch 2 times, most recently from 38047fd to 6c347ee Compare February 28, 2025 15:20
@@ -101,7 +101,8 @@
"import-in-the-middle": "^1.13.0"
},
"devDependencies": {
"@types/node": "^18.19.1"
"@types/node": "^18.19.1",
"fastify": "4.18.0"
Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid this and just vendor in the minimal interface we need to satisfy the types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure 👍

Copy link
Contributor

github-actions bot commented Feb 28, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.28 KB - -
@sentry/browser - with treeshaking flags 23.12 KB - -
@sentry/browser (incl. Tracing) 36.99 KB - -
@sentry/browser (incl. Tracing, Replay) 74.17 KB - -
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 67.55 KB - -
@sentry/browser (incl. Tracing, Replay with Canvas) 78.83 KB - -
@sentry/browser (incl. Tracing, Replay, Feedback) 90.65 KB - -
@sentry/browser (incl. Feedback) 39.68 KB - -
@sentry/browser (incl. sendFeedback) 27.9 KB - -
@sentry/browser (incl. FeedbackAsync) 32.67 KB - -
@sentry/react 25.09 KB - -
@sentry/react (incl. Tracing) 38.91 KB - -
@sentry/vue 27.51 KB - -
@sentry/vue (incl. Tracing) 38.71 KB - -
@sentry/svelte 23.32 KB - -
CDN Bundle 24.51 KB - -
CDN Bundle (incl. Tracing) 36.98 KB - -
CDN Bundle (incl. Tracing, Replay) 72.03 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) 77.18 KB - -
CDN Bundle - uncompressed 71.47 KB - -
CDN Bundle (incl. Tracing) - uncompressed 109.34 KB - -
CDN Bundle (incl. Tracing, Replay) - uncompressed 220.63 KB - -
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 233.16 KB - -
@sentry/nextjs (client) 40.53 KB - -
@sentry/sveltekit (client) 37.44 KB - -
@sentry/node 150.78 KB +5.2% +7.45 KB 🔺
@sentry/node - without tracing 96.48 KB +0.01% +1 B 🔺
@sentry/aws-serverless 120.78 KB +0.01% +8 B 🔺

View base workflow run

@onurtemizkan onurtemizkan force-pushed the onur/fastify-otel-migration branch 2 times, most recently from 88d946e to 112c80a Compare March 5, 2025 18:27
@onurtemizkan onurtemizkan marked this pull request as ready for review March 5, 2025 18:33
@andreiborza andreiborza self-assigned this Mar 10, 2025
@@ -0,0 +1,32 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

For the files we vendored in from OTEL, we should also link to the git sha we used of the versioned files.

Copy link
Member

@andreiborza andreiborza left a comment

Choose a reason for hiding this comment

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

Great job! This is looking nice.

@@ -65,6 +65,7 @@
"access": "public"
},
"dependencies": {
"@fastify/otel": "0.5.0",
Copy link
Member

Choose a reason for hiding this comment

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

l: We can bump to 0.5.1

Comment on lines 19 to 52
// /**
// * Minimal request type containing properties around route information.
// * Works for Fastify 3, 4 and presumably 5.
// *
// * Based on https://github.com/fastify/fastify/blob/ce3811f5f718be278bbcd4392c615d64230065a6/types/request.d.ts
// */
// // eslint-disable-next-line @typescript-eslint/no-explicit-any
// interface MinimalFastifyRequest extends Record<string, any> {
// method?: string;
// // since [email protected]
// routeOptions?: {
// url?: string;
// };
// routerPath?: string;
// }

// /**
// * Minimal reply type containing properties needed for error handling.
// *
// * Based on https://github.com/fastify/fastify/blob/ce3811f5f718be278bbcd4392c615d64230065a6/types/reply.d.ts
// */
// // eslint-disable-next-line @typescript-eslint/no-explicit-any
// interface MinimalFastifyReply extends Record<string, any> {
// statusCode: number;
// }

// // We inline the types we care about here
// interface Fastify {
// // eslint-disable-next-line @typescript-eslint/no-explicit-any
// version: string;
// register: (plugin: any) => Fastify;
// after: (listener?: (err: Error) => void) => Fastify;
// addHook: (name: string, handler: (...params: unknown[]) => void) => Fastify;
// }
Copy link
Member

Choose a reason for hiding this comment

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

l: Do we want to keep this around?

@onurtemizkan onurtemizkan requested review from a team as code owners April 3, 2025 14:40
@onurtemizkan onurtemizkan force-pushed the onur/fastify-otel-migration branch 2 times, most recently from 2bd1c74 to 0308803 Compare April 3, 2025 14:54
@onurtemizkan
Copy link
Collaborator Author

@andreiborza - I updated the PR. Waiting for fastify/otel#35 to be released.

@mydea
Copy link
Member

mydea commented Apr 8, 2025

I would assume this also closes #12844?

@andreiborza
Copy link
Member

andreiborza commented Apr 14, 2025

@andreiborza - I updated the PR. Waiting for fastify/otel#35 to be released.

looks like it's been released with 0.5.2 🚀

Make sure to update the lock file when you bump.

@onurtemizkan onurtemizkan force-pushed the onur/fastify-otel-migration branch 2 times, most recently from bd5f0ab to b1847dc Compare April 14, 2025 10:24
@onurtemizkan onurtemizkan force-pushed the onur/fastify-otel-migration branch from b1847dc to 275a14d Compare April 14, 2025 14:23
@onurtemizkan
Copy link
Collaborator Author

@andreiborza, @mydea. I bumped the version. I think it's ready to go now.

@onurtemizkan onurtemizkan force-pushed the onur/fastify-otel-migration branch from 43c201f to 964cf0c Compare April 14, 2025 20:31
@onurtemizkan
Copy link
Collaborator Author

I would assume this also closes #12844?

Yes, that should resolve it as well

data: {
'sentry.origin': 'manual',
'fastify.type': 'middleware',
'plugin.name': 'fastify -> @fastify/middie',
Copy link
Member

Choose a reason for hiding this comment

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

do we have no more plugin/middleware spans like this anymore? Is this "OK"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Checked again after the updates. We still have the middie spans, I added them back to the tests. But the problem is now that we don't have access to the requestHook anymore, the spans are not formatted correctly in NestJS applications (when setupFastifyErrorHandler is not used). We attempt using addFastifySpanAttributes inside setupFastifyErrorHandler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm now trying to use diagnosticsChannel for it. Maybe we can reach the context from it

Copy link
Member

Choose a reason for hiding this comment

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

can we try to PR something like the requestHook to the otel instrumentation, maybe? Or is this something they do not want?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mydea, they released a way to reach the spans: https://github.com/fastify/otel/releases/tag/v0.6.0

The problem is that with this release, they removed the vendored types we added previously. Adding fastify here as a devDependency works for TypeScript 5, but not in previous versions. Should we make this an exception for TS-3.8 support/tests?

@mydea
Copy link
Member

mydea commented Apr 15, 2025

@onurtemizkan could you maybe share a link of one or two example transactions captured to Sentry with the new version, to compare with the old version? that would be great 🙏

@onurtemizkan onurtemizkan force-pushed the onur/fastify-otel-migration branch from 5329aaa to 8ff5f09 Compare April 15, 2025 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to @fastify/otel instrumentation
4 participants