search

getsentry / sentry-javascript

Official Sentry SDKs for JavaScript
https://sentry.io
MIT License
7.89k stars 1.56k forks source link

Sentry JavaScript v7 Major Release Plan #4240

Closed AbhiPrasad closed 2 years ago

AbhiPrasad commented 2 years ago

Update 3: We have released 🚀

https://github.com/getsentry/sentry-javascript/releases/tag/7.0.0

https://www.npmjs.com/package/@sentry/browser/v/7.0.0

https://www.npmjs.com/package/@sentry/node/v/7.0.0

Update 2: There are now beta releases available to test with!

https://www.npmjs.com/package/@sentry/browser/v/next

Migration guide: https://github.com/getsentry/sentry-javascript/blob/7.x/MIGRATION.md

Update: We are now actively working towards the v7 major release. While working on it, we're "freezing" work on master.

Feel free to take a look at our v7 work here: https://github.com/getsentry/sentry-javascript/compare/master...7.x

Edit: This plan has now been turned into a proper Roadmap. Please see: https://github.com/getsentry/sentry-javascript/issues/4240#issuecomment-1035323682

Overarching issue for: https://github.com/getsentry/sentry-javascript/milestone/11

Please see: https://github.com/getsentry/sentry-javascript/compare/master...7.x

The main goals for this next major release is web bundling optimizations and increased treeshaking support. Bundle size reduction will be a side effect of these changes, but not the explicit goal.

It's also important to note that we will not be breaking any top level public APIs in this major. Anything we touch will be related to internals or to integrations/transports. Therefore unless you are using custom integrations, custom clients or monkey patching our SDK, you will have to make no code changes when upgrading the SDK.

The following is a list of the major breaking changes that we might make in the next major release. We are not committing to including all of these changes in the next release, but any feedback/thoughts are appreciated!

Treeshaking / Bundle Size

Remove Backend Class

Remove callOnHub and invokeClient methods

Extract out integrations into their own modules

Extract out transports into their own modules

Refactor client inheritance chain so that minimal methods are overridden

Refactor API class into functions

Refactor Logger class into functions

Refactor Stacktrace Parsing

Emit es6 by default

Dropping Support

Remove store endpoint support → only use envelopes

Remove @sentry/integrations package

Drop support for Node v6

Removing deprecations

Remove all references to @sentry/apm

Remove whitelistUrls and blacklistUrls options

Remove getActiveDomain

Remove tracing deprecations (startSpan and child)

Remove Gatsby SDK in window

RemoveregisterRequestInstrumentation name

Dependency upgrades

Upgrade Typescript (lol)

Upgrade to Ember 4.x / ember-auto-import 2.x

Other

Remove SentryError

Introduce a getActiveTransaction method and use it everywhere

Remove SyncPromise API

Remove @sentry/wasm in favour of just exporting an integration

Remove raven-node backward-compat code from node rejection handler

Remove forget util

Remove SDK_NAME constants in favor of SDK metadata

Inline injectReportDialog into showReportDialog and unify them

Remove helpers.ts from @sentry/browser

Introduce EventType const enum (Low Risk - Low Reward)

Remove event.stacktrace field

Improve eventID propogation

Remove tslint from @sentry-internal/typescript

Prefix interfaces with I so they clearly differ from class implementations.

AbhiPrasad commented 2 years ago

Hey everyone! For those watching the issue - here's where we are at.

We recently did a bunch of changes in prep for this major, culminating in a beta release: https://www.npmjs.com/package/@sentry/browser/v/6.17.0-beta.0. Please give it a try and let us know what you think!

After we've cut a release for the full 6.17.0 version, we'll be moving on to work on the major version. We still haven't fully formalized what exactly from the plan will end up appearing in the major, but as soon as we decide that we will update this GH issue!

yordis commented 2 years ago

Is there any opportunity to add a checkbox to every item listed?

It would help to understand what has been done already and what is missing. I think I may be able to do some of the work hopefully.

yordis commented 2 years ago

Append interfaces with I so they clearly differ from class implementations. For ex. HubI instead of Hub

Append of Prepend? Normally I would expect IHub, I am curious about that one 🤔

alfaproject commented 2 years ago

Prefixing is the traditional way for many years in .NET, Java and possibly others.

However, the TypeScript team themselves really want people to stop that practise:

yordis commented 2 years ago

@alfaproject my experience doesn't allow me to agree with such a take anymore, I understand your opinion, and totally valid.

The countless time people decided to remove the I (Including types defined using type btw nothing related to I as in interface definition only) case in point this codebase, to then realize that the name was taken, causing:

A) A lot of headaches when it comes to ambiguous language and adds a lot of cognitive loads. B) Constant renaming to avoid collisions between types and actual classes or object definitions C) Endless refactoring adding some sort of identifier (which is the case for now) such as reintroducing some sort of I prefix or suffix in order to mitigate the collision problems.

If you pay close attention, multiple projects are starting to actually move to full names for types instead of using T, P, and so on (one letter type). Wanting such good naming for the types, you will probably notice C) step where they do TData (just to make a point https://github.com/tannerlinsley/react-query/blob/6710bc1715fee2810ca3a35b76bca71738f5da7f/src/react/useBaseQuery.ts#L12-L16 they do it for generic types taking in).

So, TypeScript could suggest removing such need back then, a long time ago, but from first-hand experience maintaining all kinds of scaled software, I can't agree with them anymore. I think reading such things out of context is naive and don't think about the consequences is worrisome. I have been there, doesn't work (case in point the issue Sentry is facing).

Being said, 🤷🏻 to each their own, I don't own the codebase. But if we are honest about the situation, what I proposed wouldn't be causing issues by now and not much to explain the intention 😉

It is not an all or nothing either, don't do it because of Java or C# or whatever of those reasons, but more to fix an exciting problem.

alfaproject commented 2 years ago

No no you misunderstood me, haha. I just wanted to say no the suffix because prefix is what everyone does including for types like TData as you mentioned. What you won't see anywhere (I hope) is DataT.

The trying to avoid prefixes was just an addendum that I totally understand is not possible in any enterprise grade project. Hell we also prefix interfaces with I in some cases d:

yordis commented 2 years ago

@alfaproject sorry ... (feel stupid now missing your intention)

xuzhanhh commented 2 years ago

3271 Will this bug get fixed in v7?

AbhiPrasad commented 2 years ago

Y'all are totally right - we should be prefixing the interfaces, not appending I. Changed in the plan.

@yordis - we haven't reached the stage where we can get checkboxes going, but when we do we will update the issue!

@xuzhanhh we will address this in v7!

AbhiPrasad commented 2 years ago

Roadmap

We've spent some while experimenting, and we now have a definite road-map for the major

Before the Major

Before we start work on the major, there are a couple things we can do to ease migration, test out ideas, and set up tests. This comes in a couple stages:

Introduce New transport API + First Class Envelope Support

Our main goal here is to unblock https://github.com/getsentry/sentry-javascript/issues/4417.

We'll do this first by incrementally migrating the Transports from the old API to the new one. This means there will be a small bundle size hit to accommodate the two different transport types, but this will led to a net smaller bundle when the major comes by.

https://github.com/getsentry/sentry-javascript/pull/4527 starts work on the envelope side of things.

TraceKit Refactors

Our main goal here is to simplify TraceKit to reduce bundle size + allow for arbitrary stack trace parsers to be passed in. This means in the major, you can choose exactly what stack trace parsing you want, so if you want to remove support for safari or firefox, you can tree-shake them out! We'll also be just improving this in general by removing unnecessary intermediate types.

The awesome @timfish is helping us move forward with this! For the exact plan, see: https://github.com/getsentry/sentry-javascript/issues/4543

Introduce New integration API

We want to experiment with a new API for integrations, one that reduces bundle size and helps address some issues that are raised when a user has multiple clients/hubs (we've tracked some of those issues in GitHub)

/** Integration interface */
export interface NewIntegration {
  // bundle size efficient, represents the name
  n: string;

  // explicitly pass in hub reference so we don't worry about mutable global state
  install(hub: Hub): void;
}

Release Stability and Testing - Bundles and Node

We want to make sure we improve the testing infrastructure we have in place so the major bump goes as smooth as possible. @onurtemizkan has been doing some great work around this! We're in a good place with our current browser integration tests, now we need to do two things.

  1. Switch our playwright tests to test against different bundles (https://github.com/getsentry/sentry-javascript/issues/4546)
  2. Introduce a integration testing strategy for node (https://github.com/getsentry/sentry-javascript/issues/4260)

Bundling

In the major, we plan to start emitting different bundles (for example, a production bundle with all debug logging stripped) to give users control over what exactly they want. To accommodate this, we have the testing work detailed above, but we are also refactoring and centralizing our rollup config (along with some other smaller changes). @lobsterkatie is taking the lead with this so far.

Re-arranging types

As suggested by @yordis above, we'll be prefixing our interfaces with I for increased clarity (Session -> ISession). We'll start off by aliasing these types internally (for backwards compatibility), and making the full switch when using the major.

v7 Major

Once we have all of that in a good place, we'll be putting the SDK in a short feature freeze to focus on the major bump. At the end of each step, we'll be cutting a beta for everyone to test, and then we'll have some release candidate before final release.

Part 1: Delete Deprecations

As detailed in the original comment on this thread, we'll be going in and deleting all the deprecated code and options. This is detailed in the Removing Deprecations section on https://github.com/getsentry/sentry-javascript/issues/4240#issue-1072395896.

Part 2: Web Bundling Optimizations

Here we bump typescript, start emitting our different bundles, and switch to emitting es6 by default. If you want to use es5, we'll provide a separate npm package, and continue to produce es5 CDN bundles.

Part 3: Delete the backend class

As seen in https://github.com/getsentry/sentry-javascript/blob/master/packages/core/src/basebackend.ts. It is an unnecessary abstraction that is part of no other SDK. We'll be moving the logic into client. Early attempt made here: https://github.com/getsentry/sentry-javascript/pull/4307

Part 4: Remove support for the store endpoint + Switch to new transport API

Switch over to using the new transport API detailed in Introduce New transport API section above. Only send data to Sentry using envelopes. The envelope endpoint was added in Sentry 20.6.0, so if you are using a version of self-hosted Sentry (aka onpremise) older than that then you will need to upgrade. In addition, extract transport creation to top level Sentry.init call (instead of a client implementation detail) so they can be tree-shaken.

Part 5: Switch to new integrations API

Formalize the switch detailed in Introduce New integration API section above. In addition, extract default integration creation to top level Sentry.init call (instead of a client implementation detail) so they can be tree-shaken.

We will also extract certain integrations into their own packages:

Part 6: Refactor Class Inheritance Chain (specifically Client)

Remove all dead code that comes from overriding class methods. Leverage abstract methods when possible.

We can also maybe improve the processing event workflow, and how we create and propogate event ids (https://github.com/getsentry/sentry-javascript/issues/4571)

Part 7: Use the Angular compiler to compile @sentry/angular

See https://github.com/getsentry/sentry-javascript/issues/4644

Part 8: Final Touchups

yordis commented 2 years ago

Introduce New integration API

Could that be even shorter (by allowing compression of install)?

type HubInstall = (hub: Hub)=> void;
type Integration = [name: string, HubInstall];

Part 2: Web Bundling Optimizations

Have you checked #4381? It is a total mess but kind of proved the point if you want to save every byte possible. I am more than happy to explain the thought process if it didn't make sense.

How I feel right now since I am not part of the team.

image

smeubank commented 2 years ago

@yordis we got open source and open arms bro

dr-evil-hug

and we 100% checked 4381 I think we got the general idea of what you wanted to achieve but I am sure @AbhiPrasad would be happy to get more of your thoughts on it

AbhiPrasad commented 2 years ago

@yordis https://github.com/getsentry/sentry-javascript/pull/4381 is a great collection of ideas! It's really important to us that we minimize the public API breakage, but maybe we can do some of the class -> functions in the major.

We want to make sure we follow this as closely as possible (from the original plan at the top):

It's also important to note that we will not be breaking any top level public APIs in this major. Anything we touch will be related to internals or to integrations/transports. Therefore unless you are using custom integrations, custom clients or monkey patching our SDK, you will have to make no code changes when upgrading the SDK.

This means we can't use the changes to the scope class (because something like withScope leverages the scope class object) or the hub class, but maybe we can use some of the other ones!

With that in mind, I think we can bring in two of your changes. First, we can probably get away with converting the Session entirely to an object + functions (we'll need an incremental migration strategy though, any ideas?). Second, have any ideas on how we can do the @sentry/minimal changes (of explicitly passing in functions instead of having strings defined while keeping the hub class?

Edit:

Could that be even shorter (by allowing compression of install)?

Turning the integration into a tuple is really interesting actually. Maybe we'll give it a try when we experiment with the integration API, will ping you on the PR!

Edit 2:

We probably can't rely on a strict tuple because we want integration to be stateful (can be passed in some options in the constructor, but they don't run until they get installed). Perhaps we can just make them plain objects though.

yordis commented 2 years ago

@yordis https://github.com/getsentry/sentry-javascript/pull/4381 is a great collection of ideas! It's really important to us that we minimize the public API breakage

Generally, if you are gonna make a breaking change anyway, all it matters is the surface to go from A to B, I feel this would simplify either way, so I am biased toward breaking changes as long as people take the ownership document as much you can and provide tools and guidelines.

But don't listen to me, I am crazy when it comes to that part.

With that in mind, I think we can bring in two of your changes. First, we can probably get away with converting the Session entirely to an object + functions (we'll need an incremental migration strategy though, any ideas?). Second, have any ideas on how we can do the @sentry/minimal changes (of explicitly passing in functions instead of having strings defined while keeping the hub class?

Right right this, you can always push as much as you can to be OOP at the edge only at least. You can do it OOP from the external consumers (I think that would be the minimal thing), but in the internal codebase, everything is functional, or at least try to be.

You kind of doing that already, since you hide behind functions the fact that you eventually communicate to some global object, and people do import * as Sentry to mimic OOP but it is just modular programming.

Or at least push the OOP to that hub from type HubInstall = (hub: Hub)=> void; in the best case as well, so you don't force people to import functions to then pass hub down. Obviously, missing more opportunities to continue making things smaller. But at that point, you are probably at the end of squashing as much as you can.

Turning the integration into a tuple is really interesting actually. Maybe we'll give it a try when we experiment with the integration API, will ping you on the PR!

Be careful, at that point, what you call integration must be a tuple-of-two, so keep adding more callbacks to implement could be a problem, being said, don't be afraid, having such a simple thing shouldn't be an issue.

We probably can't rely on a strict tuple because we want integration to be stateful (can be passed in some options in the constructor, but they don't run until they get installed). Perhaps we can just make them plain objects though.

I am not sure to follow here (so probably I will explain something that you know already 😄) , but don't forget you can use closure to have "stateful" thing (cache), you can either use the file as your scope or the function closure as the scope

// sentry code
// just to hide away things and potentially giving people strong type and validation layer

type HubInstall = (hub: Hub)=> void;
type Integration = [name: string, HubInstall];

export function makeIntegration(name: string, installer: HubInstall) {
  return [name, installer];
}

const fileGlobalStuff = {}
// if you want to have config stuff and extra things or whatever your use-space integration wants.

function makeMyStatefulIntegration(someClient) {
  const anotherInternalStuff = {}
  return makeIntegration('pepeg', (hub)=> {
     // do stuff with fileGlobalStuff
     // do stuff with someClient
     // do stuff with anotherInternalStuff 
  })
}
AbhiPrasad commented 2 years ago

Generally, if you are gonna make a breaking change anyway, all it matters is the surface to go from A to B, I feel this would simplify either way, so I am biased toward breaking changes as long as people take the ownership document as much you can and provide tools and guidelines.

I agree, but we have users who rely on Sentry in many different environments, workplaces and situations, and migrating for them is often tough. It's also important to note that we haven't done a major like this in a long time - if we had a more steady cadence of majors I think we could way more easily accept these larger changes.

I think once we ship this one, we can get more comfortable with more breakage afterwards.

Right right this, you can always push as much as you can to be OOP at the edge only at least. You can do it OOP from the external consumers (I think that would be the minimal thing), but in the internal codebase, everything is functional, or at least try to be.

Totally - we adopted this pattern when converting our API and logger classes.

I am not sure to follow here (so probably I will explain something that you know already 😄) , but don't forget you can use closure to have "stateful" thing (cache), you can either use the file as your scope or the function closure as the scope

a good point - slipped my mind (in fact I did this earlier lol https://github.com/getsentry/sentry-javascript/pull/4283). I think moving to managing state in the lexical scope + tuple returns is what we probably want to target for both our transports api changes and integrations api changes.

Be careful, at that point, what you call integration must be a tuple-of-two, so keep adding more callbacks to implement could be a problem, being said, don't be afraid, having such a simple thing shouldn't be an issue.

Totally fair. I can only really see us introducing 1 other option into the integration install function, which is a destructor to essentially "uninstall the integration", I'm relatively confident in the change.

Thanks for your thoughts @yordis

BYK commented 2 years ago

@AbhiPrasad FYI for breaking but straightforward to migrate API changes, you may use CodeMods like the React team: https://github.com/reactjs/react-codemod

xr0master commented 2 years ago

Hey guys. Any ideas to rewrite the module for node.js in the 7th version?

  1. Uses the domain module that has been deprecated for many years and is not secure.
  2. The built-in express.js code, which has little to do with the node.js

Thanks.

AbhiPrasad commented 2 years ago

Hey @xr0master thanks for reaching out. We'll be taking a look at Node right after we ship the major, but removing domains is not in scope for this major version. This is tracked by https://github.com/getsentry/sentry-javascript/issues/4633.

I'm curious about this: The built-in express.js code, which has little to do with the node.js. Could you create a new GitHub issue and give more details about this? (don't want to add to this issue, which probably has a lot of watchers).

xr0master commented 2 years ago

@AbhiPrasad there is no issue, it's just that there is a lot of express.js-only code in the @sentry/node package. Maybe then it should be renamed to @sentry/express, and create a new package for the pure node server. Edit: ah, sorry. Yes, of course, I will create a separate thread.

smeubank commented 2 years ago

Hi all!

We've recently shipped our first beta for the V7 release! We would love to get your feedback on the changes we've made to the SDK and migration guide.

Thank you!

AbhiPrasad commented 2 years ago

v7 has been released! Please reach out if you have any feedback!

https://github.com/getsentry/sentry-javascript/releases/tag/7.0.0

https://www.npmjs.com/package/@sentry/browser/v/7.0.0

https://www.npmjs.com/package/@sentry/node/v/7.0.0