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.

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

@sentry/next #133

Open
buschtoens opened this issue Jun 29, 2018 · 14 comments
Open

@sentry/next #133

buschtoens opened this issue Jun 29, 2018 · 14 comments

Comments

@buschtoens
Copy link
Contributor

Sentry has been busy working on the next generation of raven.js: getsentry/sentry-javascript#1281

Yesterday they published the first beta version to npm. You can find the source code here: https://github.com/getsentry/raven-js/tree/master/packages

I just wanted to raise awareness and get the discussion going on when and how to start integrating it. :)

@Turbo87
Copy link
Collaborator

Turbo87 commented Oct 26, 2018

progress report: I've been looking into this, and for now I'm using the instructions at https://docs.sentry.io/platforms/javascript/ember/ instead of ember-cli-sentry for a few apps. At this point it is not entirely clear to me yet what advantages a dedicated addon would have over using @sentry/browser directly. I will try to assemble a list of such advantages though and if anyone has ideas please feel free to chime in.

@mydea
Copy link
Contributor

mydea commented Nov 15, 2018

I still think it would be nice to have this addon as a wrapper - there are some things this addon can/should do, even with the new Sentry SDK, imho:

  • Take care of importing the Sentry SDK
  • Provide configuration through the config/environment.js file
  • Provide a service to interact with sentry, e.g. things like .captureMessage()
  • Only register sentry in production

Basically, I think it would make sense to streamline this addon, and remove some functionality (e.g. the whole global error handling stuff), and leave it as a thin(ner) wrapper over @sentry/browser.

@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 15, 2018

* Provide configuration through the `config/environment.js` file

what's the advantage of doing that?

* Provide a service to interact with sentry, e.g. things like `.captureMessage()`

what's the advantage of doing that?

* Only register sentry in production

what's the advantage of doing that?

when I tried out @sentry/browser I also noticed that the filesize significantly increased so to be honest, right now I can't recommend moving to the new SDK unless filesize doesn't matter to you at all

@mydea
Copy link
Contributor

mydea commented Nov 15, 2018

Well, we use different sentry DSNs based on the environment, and being able to configure this in the config/environment.js is the most sensible solution for this IMHO. Of course, this is actually basically the only configuration that we really need, I guess.

Generally, I guess you could directly import sentry and call methods manually.
Even so, this could be a nice wrapper to check if sentry is not active (e.g. in development) to prevent you from putting this type of code everywhere in your app.

I guess this ties in with 1) - if no DSN is specified, sentry should not be configured.

Regarding the bundle size, that is indeed unfortunate. A quick comparison shows major differences:

raven-js: 40kb minified / 14kb gzipped
https://bundlephobia.com/result?p=raven-js@3.27.0

@sentry/browser: 100kb minified / 25kb gzipped
https://bundlephobia.com/result?p=@sentry/browser@4.3.0

Also see getsentry/sentry-javascript#1552

@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 15, 2018

Well, we use different sentry DSNs based on the environment, and being able to configure this in the config/environment.js is the most sensible solution for this IMHO. Of course, this is actually basically the only configuration that we really need, I guess.

there is nothing keeping you from referencing the config during the init() call of @sentry/browser

Generally, I guess you could directly import sentry and call methods manually.

yep, and that's the official way of using their SDK.

Even so, this could be a nice wrapper to check if sentry is not active (e.g. in development) to prevent you from putting this type of code everywhere in your app.

if the SDK is not active the function calls will do nothing, meaning they won't throw exceptions either, so there is no need for defensive programming here

I guess this ties in with 1) - if no DSN is specified, sentry should not be configured.

see above. @sentry/browser supports this "disabled" mode by default.

@mydea
Copy link
Contributor

mydea commented Nov 15, 2018

All good points - but I still think having a thin wrapper addon to take care of 1) would make sense. Of course I, and everbody else, could do this manually on their own, but it's nice to have an addon take care of that - as this code will probably look pretty much the same for everbody, so why not have a common solution for it.

I guess it would then be mostly a shim, with potentially an initializer to do the init stuff... But as you said, as long as they don't have a solution for the exploding size, it's probably not worth migrating anyhow.

@tchak
Copy link
Contributor

tchak commented Feb 12, 2019

What about fastboot support? Will @sentry/browser work in fastboot environment? Or is there a still a need to load conditionally @sentry/node and provide a different set of API keys?

@lindyhopchris
Copy link

@Turbo87 would be interested to hear what your current thoughts are on this. I've previously used this addon, but when adding Sentry to a new app I've gone with @sentry/browser to give it a go.

One of the things I really like is I can set up Sentry in app.js before the application is exported and created. This presumably solves #164 (but need to check that).

I've got the following file:

// app/sentry.js
import * as Sentry from '@sentry/browser'
import * as Integrations from '@sentry/integrations';
import { get } from '@ember/object';
import { assign } from '@ember/polyfills';
import config from './config/environment';

Sentry.init(assign({
  integrations: [new Integrations.Ember()],
  release: get(config, 'APP.version'), // ember-cli-app-version
  environment: config.environment
}, config.sentry || {}));

Then in my app.js I've added:

import './sentry';

I don't use Fastboot but I can see it might be useful to have an addon that provides a version of the sentry.js above that loads either Sentry browser or node depending on whether it's a Fastboot or Browser environment. Then in your app.js you'd just need to add something like:

import 'ember-cli-sentry/init';

The only point of a service I can see is to provide a single place where you decide whether an exception should be reported, e.g. if you don't want abort errors to be reported.

@Turbo87
Copy link
Collaborator

Turbo87 commented Apr 12, 2019

@lindyhopchris I had ported one project to use @sentry/browser, but then I noticed that their SDK is three times the size of raven-js which prevented me from ever merging that PR in. there is an issue on their project about the project size, but afaik it's not resolved yet. until that is the case I would recommend staying with raven-js unless you a) need some of the new features or b) don't need to care about the shipped file size

@lindyhopchris
Copy link

Ah ok, I thought the size had been dropped for v5:
https://bundlephobia.com/result?p=@sentry/browser@5.0.8

However, I'm at (b) - I'm using it in internal apps where we don't care about shipped file size, so not really for me to say. Will keep watching this issue for whenever it's deemed the right size for use!

@Turbo87
Copy link
Collaborator

Turbo87 commented Apr 12, 2019

oh, interesting! I admit that it had been a while since I last looked at it. with the v5 size it seems a lot more viable to upgrade now.

@josemarluedke
Copy link

Now with v5, it would be nice to have this revisited.

@mapohjola
Copy link

Is there any plans for this or should we be moving away from this repo?

@Ramblurr
Copy link

FYI for future visitors: It seems like sentry has produced their own ember addon:

https://github.com/getsentry/sentry-javascript/tree/master/packages/ember

I have yet to try it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants