sentry-javascript
sentry-javascript copied to clipboard
getsentry
feat(remix): Add Fastify server adapter
Before submitting a pull request, please take a look at our Contributing guidelines and verify:
- [x] If you've added code that should be tested, please add tests.
- [x] Ensure your code lints and the test suite passes (
yarn lint) & (yarn test).
Hello, thank you for this PR!
Actually, I wonder if we need this at all in v8 - there, we auto-instrument fastify & express under the hood, so maybe we can get rid of this actually 🤔 needs some more testing though, I guess - any thoughts @onurtemizkan & @AbhiPrasad ? I am not so familiar with Remix systems and how these parts work together...!
Hello, thank you for this PR!
Actually, I wonder if we need this at all in v8 - there, we auto-instrument fastify & express under the hood, so maybe we can get rid of this actually 🤔 needs some more testing though, I guess - any thoughts @onurtemizkan & @AbhiPrasad ? I am not so familiar with Remix systems and how these parts work together...!
My pleasure! I am now setting up things for e2e manual test. As for the wrapper itself, what I have learned so far from the codebase, we need to know ahead of time how to parse out details from the request data. We also need to know the name of the response's end method, to flush the data to remote repo prior to resolving
Actually, I wonder if we need this at all in v8 - there, we auto-instrument fastify & express under the hood, so maybe we can get rid of this actually 🤔 needs some more testing though, I guess - any thoughts @onurtemizkan & @AbhiPrasad ? I am not so familiar with Remix systems and how these parts work together...!
@mydea, yes I was also curious about it, had some trials to remove adapters, but looks like we need to keep those for the time being.
I would think that with v8, where we have native support for fastify, this should not be necessary anymore...? Did you have the chance to try it out, so using @sentry/remix v8 out of the box, with no special fastify code?
I would think that with v8, where we have native support for fastify, this should not be necessary anymore...? Did you have the chance to try it out, so using
@sentry/remixv8 out of the box, with no special fastify code?
I think I got a déjà vu and it's super confusing :confused:
I remember we agreed these changes were necessary, and we got them implemented and tested. If that's not needed in sentry@v8 why would we still have express specific test apps in the dev-packages? Should they be swapped for remix and remix-vite?
I am super ok if these changes are not needed, but can the maintainers then close this PR with a corresponding comment, so that we gracefully finish our work? Leaving it behind this way with no feedback is not super motivating for people to contribute to the project :wink:
yeah sorry about this, I get that this is confusing, I am a bit confused myself 😅
@onurtemizkan recently opened a PR to use a new remix instrumentation: https://github.com/getsentry/sentry-javascript/pull/12110
@onurtemizkan could you chime in if that PR would supersede/"fix" what this PR here has set out to do? Or if that will still be relevant?
Sorry for the late response @rustworthy and @mydea. I've been trying to recover from jet lag 😅.
Ideally, https://github.com/getsentry/sentry-javascript/pull/12110 should make Fastify covered out of the box, like Express. That PR is the result of an experiment that turned out to work fine to replace the Express server adapter. So I would expect it to work for Fastify as well. Though, I did not have the bandwidth to test Fastify on that branch. We still may need a part of the updates in this PR (at least the new tests for sure), so I'd suggest keeping this PR open until we finalise #12110 and validate the Fastify usage if that makes sense to you guys. If everything goes well on that side, we can merge in the tests here.