feat: Add @sentry/ember #2739

k-fish · 2020-07-13T21:01:36Z

Summary

This will add the @sentry/ember package. This will provide an Ember addon that fits into the Ember ecosystem to replace the existing error reporting, and will in the future offer custom framework specific functionality. Once #2736 and other tracing PR's have landed, we can add Ember specific performance instrumentation.

WIP - let me know if the broad strokes of this look correct

Other

Most of the files here come from Ember addon boilerplate, which in most cases is best to leave as it makes upgrading anything Ember related easier, usually with some quick steps via code-mods.
Since it's an Ember addon and uses Embers build system, it shouldn't need to be pre-compiled
Added a dummy application

Dummy Application

To test out and make sure using GlobalEventProcessor suffices in lieu of Ember's error hooks, as a lot has changed, RSVP is now replaceable with Promise etc., to do so added an instrumented app as the dummy test application. This can be used to give examples of instrumentation as well as directly via Ember's acceptance testing.

Todo

Confirm GlobalEventProcessor is sufficient in lieu of onerror.
If this approach is fine, add acceptance tests
~~Ensure compatibility with v2 addon spec~~ Later when v2 addons are backwards compatible
~~Check that it's tree-shakeable to reduce concerns about package size and it's impact on bundle.~~ Later when v2 addons are backwards compatible
~~Add Ember (and potentially Ember-data) versions as context~~ Leaving this out for now, not really that useful.

This will add the `@sentry/ember` package. This will provide an Ember addon that fits into the Ember ecosystem and will in the future offer custom framework specific functionality. Once #2736 and other tracing PR's have landed, we can add Ember specific performance instrumentation.

getsentry-bot · 2020-07-13T21:08:11Z

	Messages
📖	@sentry/browser bundle gzip'ed minified size: (ES5: 17.2529 kB) (ES6: 16.3672 kB)
📖	✅ TSLint passed

Generated by 🚫 dangerJS against 3984198

HazAT · 2020-07-14T10:48:06Z

packages/ember/LICENSE

@@ -0,0 +1,29 @@
+BSD 3-Clause License


We should use MIT license, forgot to say this for react :/

Yeah it was MIT but I switched it because of react, 👍

AbhiPrasad · 2020-07-14T12:39:59Z

packages/ember/.editorconfig

@@ -0,0 +1,19 @@
+# EditorConfig helps developers define and maintain consistent
+# coding styles between different editors and IDEs
+# editorconfig.org


Can we discuss this as a global change, IMO we should be discouraging per package editor config + the whitespace and newline stuff should be taken care of by prettier.

Generated 😄. I'm fine with using other config to align with the mono-repo, but this is the config that pretty much every Ember addon uses (once again, strong conventions).

Hmm. Alright lets keep it then, but defer to @HazAT for any thoughts.

Yeah, I am also a bit torn here, I would prefer not to have this.
If we want to have this, we should intro this mono repo wide (which should be another PR) so please remove it for now.

AbhiPrasad · 2020-07-14T12:40:40Z

packages/ember/.ember-cli

+
+    Setting `disableAnalytics` to true will prevent any data from being sent.
+  */
+  "disableAnalytics": false


cool TIL. Maybe we should do something similar for sentry-cli @kamilogorek (unless this already exists).

We don't send any analytics in sentry-cli afaik?

Ignore my previous comment then 😅

Last I remember it uses leek to send build metrics to GA. It lets the Ember team know how addons and apps are being built and issues with the pipeline so they can design against actual usage vs. theoretical.

AbhiPrasad · 2020-07-14T12:42:40Z

packages/ember/.eslintrc.js

+        '.eslintrc.js',
+        '.template-lintrc.js',
+        'ember-cli-build.js',
+        'index.js',


Wait why the eslint config? is it just autogenerated?

I know tslint sucks, and we are looking to update to eslint soon.

Most of this is auto-generated for Ember addons, it's IMO best to leave addon settings as they are all built the same, which encourages community support. Upgrading for changes to ts/es linting rules in the future is simpler too as there are usually simple migration paths for most things if you follow Ember conventions.

Gotcha lets keep it then.

packages/ember/addon/index.ts

packages/ember/tests/dummy/app/components/test-section.hbs

…tryForEmber is called

…ror, and adjust config so that config for Sentry is separate, similar to how other Ember 'wrapper' addons use config

github-actions · 2020-07-21T16:01:10Z

size-limit report

Path	Size
@sentry/browser - CDN Bundle (gzipped)	17.66 KB (0%)
@sentry/browser - Webpack	18.42 KB (0%)
@sentry/react - Webpack	18.42 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped)	22.64 KB (0%)

…ev mode when testing the dummy application

… 3.12-LTS

…included instead of special casing transport

HazAT · 2020-07-23T10:32:30Z

packages/ember/package.json

+  "repository": "git://github.com/getsentry/sentry-javascript.git",
+  "homepage": "https://github.com/getsentry/sentry-javascript/tree/master/packages/ember",
+  "author": "Sentry",
+  "license": "BSD-3-Clause",


Suggested change

"license": "BSD-3-Clause",

"license": "MIT",

HazAT · 2020-07-23T10:36:33Z

packages/ember/.editorconfig

@@ -0,0 +1,19 @@
+# EditorConfig helps developers define and maintain consistent
+# coding styles between different editors and IDEs
+# editorconfig.org


Yeah, I am also a bit torn here, I would prefer not to have this.
If we want to have this, we should intro this mono repo wide (which should be another PR) so please remove it for now.

HazAT · 2020-07-23T10:40:42Z

packages/ember/.npmignore

@@ -0,0 +1,32 @@
+# compiled output


Kind of nitpicking but all our other npmignore files look something like:

* !/dist/**/* !/esm/**/*

which is far easier to understand and also less error prone. So instead of adding everything and ignoring specific things, we do the opposite of, first ignore everything and then we pick what we actually want.

I guess this is some kind of ember boilerplate, but are we may be able to come up with something similar?

No problem, I don't mind going through the effort of changing it, but the npmignore is generated by the blueprint for Ember addons, which gets updated if the file layout ever changes (which it might soon enough). Every other Ember addon uses the same .npmignore. If any updates to the file structure happen we can simply just run the blueprint command again and use the new npmignore without having to go in to determine which of the files may or may not be used by the addon ecosystem in some way.

tldr; leaving as is is more maintainable, as the file structure for Ember addons is convention based.

HazAT · 2020-07-23T10:42:00Z

packages/ember/README.md

+}
+```
+
+### Additional Configuration


Should we mention something here about our existing ember integration in @sentry/intergrations?
Like this package is the successor or something.

Should it be here in additional configuration or above under general?

HazAT · 2020-07-23T10:43:08Z

packages/ember/package.json

+    "@sentry/browser": "^5.20.0",
+    "@sentry/types": "^5.20.0",
+    "@sentry/utils": "^5.20.0",


Suggested change

"@sentry/browser": "^5.20.0",

"@sentry/types": "^5.20.0",

"@sentry/utils": "^5.20.0",

"@sentry/browser": "5.20.0",

"@sentry/types": "5.20.0",

"@sentry/utils": "5.20.0",

These should be fully version locked, otherwise lerna has troubles.

HazAT · 2020-07-23T10:45:52Z

packages/ember/tsconfig.json

+    "target": "es2017",
+    "allowJs": true,
+    "moduleResolution": "node",
+    "allowSyntheticDefaultImports": true,
+    "noImplicitAny": true,
+    "noImplicitThis": true,
+    "alwaysStrict": true,
+    "strictNullChecks": true,
+    "strictPropertyInitialization": true,
+    "noFallthroughCasesInSwitch": true,
+    "noUnusedLocals": true,
+    "noUnusedParameters": true,
+    "noImplicitReturns": true,
+    "noEmitOnError": false,
+    "noEmit": true,
+    "inlineSourceMap": true,
+    "inlineSources": true,
+    "baseUrl": ".",
+    "module": "es6",
+    "experimentalDecorators": true,


Can we somehow reduce this and align it with other tsconfig.json we have in other packages.
This file should really only contain what absolutely necessary and ideally, extend our root tsconfig.json like other packages.

I'll take a look 👍

I've extended and reduced it like you mentioned, but again Ember is conventional with which typescript it uses (modules are aimed at es6 and decorators are strongly encouraged to use in Octane). Guidance around settings in Ember is to usually leave defaults unless you have a very specific reason to change them, and rely on tooling (automated cli tools, codemods, etc) to upgrade and update.

Makes sense. What version of Ember will this support then?

All supported LTS versions. They are tested with ember-try (see here: https://github.com/getsentry/sentry-javascript/pull/2739/files#diff-7545849fb3cdafe10c02f667d5efe2f0), I'll add a note to the README though.

AbhiPrasad

Looks good!

packages/ember/LICENSE

packages/ember/README.md

AbhiPrasad · 2020-07-23T16:44:01Z

packages/ember/README.md

+import config from './config/environment';
+import { SentryForEmber } from '@sentry/ember';
+
+SentryForEmber();


Can we make this InitSentryForEmber() to make it more explicit on what is going on?

Nice, will do.

AbhiPrasad · 2020-07-23T16:48:13Z

packages/ember/tests/acceptance/sentry-errors-test.js

+import { find, click, visit } from '@ember/test-helpers';
+import { run } from '@ember/runloop';
+import Ember from 'ember';
+import sinon from 'sinon';


Can we make sure we use jest for everything? I know this might be an ember default, but I would like to prevent multiple testing libraries.

It's not really just a default, it's pretty much the only testing library that is tightly integrated with Ember. I prefer jest over qunit in general, but for Ember it's not nearly as nice to setup with other frameworks (the only other real option being Mocha) as the other helpers etc. are missing. We'd have to do a fair amount of groundwork to get it working with Jest.

On the other hand though, the guides and documentation are great for Ember, and aside from a few more tests for performance / profiling I don't imagine we'll have many to maintain.

Ok I guess best tool for the job kind of thing. Let's keep with the defaults then 👍

AbhiPrasad · 2020-07-23T16:51:31Z

.github/workflows/sentry-ember.yml

+        run: yarn run lint --scope=${{env.lerna-package}}
+
+      - name: Build dependent packages
+        run:  yarn build --ignore="@sentry/integrations" --ignore="@sentry/ember" --ignore="@sentry/react" --ignore="@sentry/apm" --ignore="@sentry/gatsby" --ignore="@sentry/node"


I think @HazAT just converted a bunch to GHA, can we just leverage those and add ember under test?

We don't need to duplicate lint and build that way.

I'll take a look, 👍

Yeah took a look at the the GHA workflow and removed this, it's a lot simpler to just run the build for everything, it probably isn't worth dealing with managing individual workflows for a small speed increase.

AbhiPrasad · 2020-07-23T16:52:43Z

packages/ember/addon/index.ts

+    }
+    next(null, function () {
+      warn(
+        'Ember.onerror found. Using Ember.onerror can hide some errors (such as flushed runloop errors) from Sentry. Use Sentry.captureException to capture errors within Ember.onError or remove it to have errors caught by Sentry directly. This error can be silenced via addon configuration.',


Interesting. So we ask users to instrument Ember.onError theirselves? Any reason we didn't opt for automatic instrumentation of Ember.onError?

Not quite, Ember.onerror doesn't fully cover all error, just some of the errors within Ember. If someone doesn't have Ember.onerror declared, then all errors will naturally forward to Sentry. If they do have onerror declared and are doing something with those errors, then a subset of errors might be getting ignored and not forwarded to Sentry, hence the warning.

Oh, I misunderstood, that makes much more sense. Cool 👍

AbhiPrasad

HazAT

🥇

This will add the `@sentry/ember` package. This will provide an Ember addon that fits into the Ember ecosystem and will in the future offer custom framework specific functionality.

k-fish requested a review from kamilogorek as a code owner July 13, 2020 21:01

AbhiPrasad self-requested a review July 13, 2020 21:03

HazAT reviewed Jul 14, 2020

View reviewed changes

AbhiPrasad reviewed Jul 14, 2020

View reviewed changes

k-fish added 6 commits July 14, 2020 09:26

Fix license to be MIT

b8948b0

Remove side-effect and only create the Ember event processor when Sen…

0254fb1

…tryForEmber is called

Add acceptance tests that assert calls to Sentry are being made

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

e838aec

Add warning for onerror to explain how it interacts with Sentry errors

36ddff7

Allow addon-specific config to turn off warn message about Ember.oner…

0fb3711

…ror, and adjust config so that config for Sentry is separate, similar to how other Ember 'wrapper' addons use config

Merge remote-tracking branch 'origin/master' into feat/sentry/ember

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

a5bfa53

k-fish added 8 commits July 21, 2020 09:52

Update package.json for @sentry/browser 5.20.0

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

20466eb

Add changelog entry

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

4c22bc0

Add to lerna packages

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

6e489e3

Use a custom transport so that acceptance tests work in both CI and D…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

4a03dc5

…ev mode when testing the dummy application

Switch components back to pre-octane so that they are compatible with…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

7441988

… 3.12-LTS

Cleanup template lint / eslint

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

f0779dd

Ignore ember in additional node.js checks

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

55f35fe

Ignore @sentry/ember in build phase as well as node > 10 is required

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

2b7134d

k-fish changed the title ~~[WIP] feat: Add @sentry/ember~~ feat: Add @sentry/ember Jul 22, 2020

Fix typing around passing config at runtime, so that all options are …

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

98aaecb

…included instead of special casing transport

k-fish force-pushed the feat/sentry/ember branch 4 times, most recently from 7b4240d to 1734c00 Compare July 22, 2020 20:48

Use Github Actions

6adacee

k-fish force-pushed the feat/sentry/ember branch from 1734c00 to 6adacee Compare July 22, 2020 21:02

Remove extra click / console log call

fcd2391

HazAT requested changes Jul 23, 2020

View reviewed changes

AbhiPrasad reviewed Jul 23, 2020

View reviewed changes

k-fish added 7 commits July 23, 2020 15:57

Address some PR feedback

2b50a53

Switch naming of init function for clarity

db0354f

Extend base tsconfig and remove overlapping config

adba304

Remove GHA package-specific workflow

06a1e8f

Update to 5.20.1

967c60e

Merge remote-tracking branch 'origin/master' into feat/sentry/ember

29c8ea5

Added note about previous integration

7e8a175

AbhiPrasad approved these changes Jul 27, 2020

View reviewed changes

AbhiPrasad requested a review from HazAT July 27, 2020 16:17

Add supported versions

565189f

HazAT approved these changes Jul 28, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into feat/sentry/ember

3984198

AbhiPrasad merged commit 9428c9a into master Jul 28, 2020

AbhiPrasad deleted the feat/sentry/ember branch July 28, 2020 15:47

feat: Add @sentry/ember #2739

feat: Add @sentry/ember #2739

Conversation

k-fish commented Jul 13, 2020 • edited Loading

Summary

Other

Dummy Application

Todo

getsentry-bot commented Jul 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 21, 2020 • edited Loading

size-limit report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AbhiPrasad left a comment

Choose a reason for hiding this comment

HazAT left a comment

Choose a reason for hiding this comment

k-fish commented Jul 13, 2020 •

edited

Loading

getsentry-bot commented Jul 13, 2020 •

edited

Loading

github-actions bot commented Jul 21, 2020 •

edited

Loading

AbhiPrasad left a comment •

edited

Loading