feat(core,node-core): Consolidate bun and node types with ServerRuntimeOptions #18734
Conversation
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Lms24
left a comment
There was a problem hiding this comment.
Nice, thanks for adding! Had a naming suggestion and I have another question: What about Cloudflare and Vercel edge? Both are based on the ServerRuntimeClient so I'm wondering if we can use these options as well. Also, both are WinterTC compliant 😅
Not saying the other runtimes have to happen in this PR. I'm more interested in the general approach we're taking here.
| * | ||
| * @see https://wintercg.org/ | ||
| */ | ||
| export interface BaseWinterTCOptions { |
There was a problem hiding this comment.
apologies for the naming bike shedding in advance 😅 Feel free to disregard/overrule me but should we call this something more user-friendly? Something like ServerRuntimeOptions?
This would match the ServerRuntimeClientOptions we already have for the respective client.
There was a problem hiding this comment.
Yeah I would 2nd this. WinterTC hasn't been around for long and might change name again!
There was a problem hiding this comment.
That makes total sense. I like it more than WinterTCOptions
You are absolutely right, theoretically also Deno right?
Once this PR lands I'll do a follow up right with the other runtimes/SDKs. |
fb8136c to
569cc6f
Compare
569cc6f to
b2241a1
Compare
|
@Lms24 as a quick update. The types had to be moved from |
packages/node-core/src/types.ts
Outdated
| * | ||
| * @see https://wintercg.org/ | ||
| */ | ||
| export interface ServerRuntimeOptions { |
There was a problem hiding this comment.
As discussed offline, let's ensure that ServerRuntimeOptions only includes the options that can be used across all SDKs using or extending from ServerRuntimeClient.
For the remaining options shared by Node and Bun, we can do whatever makes the most sense. Happy to leave this up to you.
Thanks! :)
There was a problem hiding this comment.
I updated the description. The new option with OTel support is now called OpenTelemetryServerRuntimeOptions
Lms24
left a comment
There was a problem hiding this comment.
Nice, thanks for reworking the hierarchy!
closes #18437
closes JS-1272
This adds a new type
ServerRuntimeOptionsinside@sentry/core, a type which can be used for all SDKs without OpenTelemetry suppport. In case OpenTelemetry support is neededOpenTelemetryServerRuntimeOptionsare exported from@sentry/node-core, which extendsServerRuntimeOptionswith the options which are needed to support OTel.For now we don't have a nice testing strategy for Bun yet, and I didn't want to copy paste all Node integration/e2e tests just for this, I'm still up for suggestions.