feat(GUI): use `resin-corvus` in AnalyticsService #1208

stefan-mihaila · 2017-03-23T17:29:46Z

This commit also removes bower.

Change-Type: patch
Fixes: #1027

jviotti · 2017-03-23T19:14:35Z

@stefan-mihaila This line from the Makefile (GitHub doesn't allow me to comment on it):

$(foreach prerequisite,$^,$(call execute-command,cp -RLf $(prerequisite) $@))

Can be simplified to cp -Rlf $< $@ now that we removed bower.

jviotti · 2017-03-23T19:16:43Z

I think we should move utils.hideAbsolutePathsInObject to resin-raven as well.

jviotti · 2017-03-23T19:18:57Z

We should probably get rid of the following TrackJS configuration: https://github.com/resin-io/etcher/blob/master/lib/gui/index.html#L15. Note that

console: {
    error: false
}

is important to prevent multiple errors (#1176), so I think we should move that configuration setting to resin-raven, in case Sentry doesn't do that already.

jviotti · 2017-03-23T19:20:13Z

This TrackJS script import should go away as well: https://github.com/resin-io/etcher/blob/master/lib/gui/index.html#L23, along with the trackjs module from package.json

jviotti · 2017-03-23T19:22:30Z

We pass the application name to TrackJS here: https://github.com/resin-io/etcher/blob/master/lib/gui/index.html#L17. Is that needed for Sentry? Should resin-raven accept that if so?

jviotti · 2017-03-23T19:23:44Z

lib/gui/modules/analytics.js

   *
   * @param {String} message - message
   *
   * @example
   * AnalyticsService.log('Hello World');
   */
  this.logDebug = (message) => {
-    const debugMessage = `${new Date()} ${message}`;
-
-    if (SettingsModel.get('errorReporting') && isRunningInAsar()) {


Is deleting SettingsModel.get('errorReporting') indented? How does disabling error reporting from the settings page works now?

It wasn't intended. I did this now: https://github.com/resin-io/etcher/pull/1208/files#diff-05cd97d7d3334a665e1467c4c5d81043R90, let me know what you think. I do need to better test it though.

jviotti · 2017-03-23T19:25:09Z

lib/gui/modules/analytics.js

@@ -130,7 +68,7 @@ analytics.service('AnalyticsService', function($log, $window, $mixpanel, Setting
   * @public
   *
   * @description
-   * This function sends the debug message to TrackJS and Mixpanel.
+   * This function sends the debug message to Mixpanel and Sentry.


I think we can isolate Etcher from the services being used. Something like "THis function sends the debug message to error reporting services" should suffice.

jviotti · 2017-03-23T19:25:39Z

lib/gui/modules/analytics.js

@@ -141,21 +79,7 @@ analytics.service('AnalyticsService', function($log, $window, $mixpanel, Setting
   * });
   */
  this.logEvent = (message, data) => {
-    const debugData = utils.hideAbsolutePathsInObject(utils.makeFlatStartCaseObject(data));
-
-    if (SettingsModel.get('errorReporting') && isRunningInAsar()) {


Again here, how do we ensure analytics are not sent anywhere if the user disabled error reporting?

jviotti · 2017-03-23T19:26:06Z

lib/gui/modules/analytics.js

@@ -141,21 +79,7 @@ analytics.service('AnalyticsService', function($log, $window, $mixpanel, Setting
   * });
   */
  this.logEvent = (message, data) => {
-    const debugData = utils.hideAbsolutePathsInObject(utils.makeFlatStartCaseObject(data));


Hm, it looks like you even deleted utils.hideAbsolutePathsInObject here. Is this function completely unused now? Should we move it to resin raven?

jviotti · 2017-03-23T19:26:42Z

lib/gui/modules/analytics.js

@@ -164,7 +88,7 @@ analytics.service('AnalyticsService', function($log, $window, $mixpanel, Setting
   * @public
   *
   * @description
-   * This function logs an exception in TrackJS.
+   * This function logs an exception in Sentry.


Again, lets say something generic like "this function logs an exception to an error reporting service" or something like that.

jviotti · 2017-03-23T19:27:51Z

lib/gui/modules/analytics.js

      errors.shouldReport(exception)
    ])) {
-      $window.trackJs.track(exception);
+      resinRaven.logException(exception);


I noticed .logException also prints the error, so the $log.error(exception) would be redundant. Maybe we should only call $log.error(exception) on an else block on this conditional?

stefan-mihaila · 2017-03-23T23:25:51Z

so I think we should move that configuration setting to resin-raven, in case Sentry doesn't do that already.

Sentry doesn't by default print the errors to the console

We pass the application name to TrackJS here: https://github.com/resin-io/etcher/blob/master/lib/gui/index.html#L17. Is that needed for Sentry? Should resin-raven accept that if so?

It is not needed.

jviotti · 2017-03-24T15:25:53Z

Sentry doesn't by default print the errors to the console

I mean, TrackJS will intercept stderr output on DevTools, and pipe it to the service as errors. Can you confirm that's not the case with Sentry? For example, when using TrackJS, if I do console.error('foo') on DevTools, then there will be a "foo" error on TrackJS.

jviotti · 2017-03-27T21:03:34Z

lib/gui/modules/analytics.js


  /**
   * @summary Log a debug message
   * @function
   * @public
   *
   * @description
-   * This function sends the debug message to TrackJS only.
+   * This function sends the debug message to error reporting services.
   *
   * @param {String} message - message
   *
   * @example
   * AnalyticsService.log('Hello World');
   */
  this.logDebug = (message) => {


I noticed logDebug, logEvent, and logException map exactly to resinCorvus's functions, so maybe we can do:

this.logDebug = resinCorvus.logDebug; this.logEvent = resinCorvus.logEvent; this.logException = resinCorvus.logException;

jviotti · 2017-03-27T21:04:57Z

lib/gui/modules/analytics.js

+   * @example
+   * AnalyticsService.disable()
+   */
+  this.disable = () => {


What happens if I send an event/exception/etc when resinCorvus is disabled? Is it a no-op?

It isn't a no-op. It will print the exception to the console, but not send it to external services.

jviotti · 2017-03-27T21:05:39Z

lib/gui/modules/analytics.js

+   * @example
+   * AnalyticsService.enable()
+   */
+  this.enable = () => {


Is resinCorvus enabled by default? Maybe we should document that somewhere in this file? If its not enable by default, maybe we should check the settings at startup and configure accordingly?

Also, can we ensure that if resinCorvus is enabled by default, and the user has disabled error reporting in the settings, then we don't send any analytics until ensuring that's what the user wants?

I think that other code might be evaluated before this, causing the startup events to be sent before we realise that analytics reporting shouldn't happen.

You're right. Using Store.subscribe(callback) doesn't solve the issue, because by the time the callback is called, we already log events.

Besides checking the setting in analytics before sending events, like we did before, I don't have any good ideas.

Besides checking the setting in analytics before sending events, like we did before, I don't have any good ideas.

That is the best approach I can think of as well, lets go for it!

jviotti · 2017-03-27T21:08:34Z

lib/gui/pages/settings/controllers/settings.js

@@ -86,6 +86,14 @@ module.exports = function(WarningModalService, SettingsModel, ErrorService, Anal
      dangerous
    });

+    if (setting === 'errorReporting') {


I feel we shouldn't put logic on how to react to certain setting here, and that we should query the redux store for the value instead. Maybe we can add another .run() block at app.js that subscribes to the store and checks the setting there? e.g: like https://github.com/resin-io/etcher/blob/master/lib/gui/app.js#L132

stefan-mihaila · 2017-03-27T23:19:39Z

I mean, TrackJS will intercept stderr output on DevTools, and pipe it to the service as errors. Can you confirm that's not the case with Sentry? For example, when using TrackJS, if I do console.error('foo') on DevTools, then there will be a "foo" error on TrackJS.

Oh, now I understand what you are saying. I tried it now, and Sentry doesn't do that.

stefan-mihaila · 2017-03-31T12:47:02Z

Just to confirm: are we waiting for something on resin-corvus side before we can start making use of this?

No, the version of resin-corvus used by this PR is published.

jviotti · 2017-03-31T18:32:18Z

Approved! There are some conflicts that we need to resolve before merging.

stefan-mihaila · 2017-04-03T09:59:44Z

@jviotti Resolved conflicts

jviotti · 2017-04-04T18:11:40Z

@stefan-mihaila Looks like there are conflicts again :P Also, the builds are failing because child-writer requires a dependency that you removed. I guess we'll have to keep that one for now.

@lurch Looks like your dependency check scripts have proved their value :D Any clues about why they don't fail on Appveyor though? See:

stefan-mihaila · 2017-04-06T20:50:32Z

@jviotti @lurch Rebased and added the culprit dependency (thanks @lurch)

Looks like ensure-all-node-requirements-available.shdoesn't play nice with require('resin-corvus/browser') though?

./scripts/ci/ensure-all-node-requirements-available.sh
lib/gui/modules/analytics.js requires 'resin-corvus/browser' module which can't be located

jviotti · 2017-04-06T21:08:39Z

@stefan-mihaila Looks like we should modify the script to only consider whatever its inside require() until the first slash.

jviotti · 2017-04-06T21:09:36Z

I'm not 100% familiar with this script, but I think changing this regular expression will do the trick: https://github.com/resin-io/etcher/blob/master/scripts/ci/ensure-all-node-requirements-available.sh#L41

lurch · 2017-04-07T12:05:07Z

Hello! Apologies for the delayed response...

@lurch Looks like your dependency check scripts have proved their value :D

Haha, cool! :-D

Any clues about why they don't fail on Appveyor though?

Looks like my script on Appveyor did 'spot' the error:

./scripts/ci/ensure-all-node-requirements-available.sh
lib/child-writer/renderer-utils.js requires 'electron-is-running-in-asar' module which can't be located
lib/gui/modules/analytics.js requires 'resin-corvus/browser' module which can't be located

but for some reason it didn't cause the entire CI build to fail. I'll have to investigate...

Looks like ensure-all-node-requirements-available.sh doesn't play nice with require('resin-corvus/browser') though?

Hmm okay, so what does require('resin-corvus/browser') signify? Should I just be checking that resin-corvus is listed in the dependencies in package.json ?

@stefan-mihaila Looks like we should modify the script to only consider whatever its inside require() until the first slash.

Unfortunately it's not quite as simple as that, as there's also lots of 'local modules' used by Etcher, e.g. https://github.com/resin-io/etcher/blob/master/lib/start.js#L33 or https://github.com/resin-io/etcher/blob/master/lib/gui/models/flash-state.js#L28 which my script does also check for and validate.

So what's the syntax used by require() to determine whether require('resin-corvus/browser') means to look for an installed module named resin-corvus, or whether to look for a local file named resin-corvus/browser.js ? Is it simply the case that all 'local requires' must start with ./ or ../ ?

lurch · 2017-04-07T12:14:38Z

lib/gui/modules/analytics.js

- *   console.log('Host architecture is x64');
- * }
- */
-const getHostArchitecture = () => {


Oh? How come this isn't needed any more?

It was moved to resin-corvus

lurch · 2017-04-07T12:16:04Z

Makefile

@@ -189,18 +201,27 @@ $(BUILD_DIRECTORY)/node-$(TARGET_PLATFORM)-$(TARGET_ARCH)-dependencies/node_modu
 		-t node \
 		-s "$(TARGET_PLATFORM)"

-$(BUILD_DIRECTORY)/electron-$(TARGET_PLATFORM)-$(TARGET_ARCH)-dependencies/bower_components: bower.json \
-	| $(BUILD_DIRECTORY)/electron-$(TARGET_PLATFORM)-$(TARGET_ARCH)-dependencies
-	./scripts/build/dependencies-bower.sh -p -x $|


lurch · 2017-04-07T12:17:16Z

Makefile

 	./scripts/build/electron-create-resources-app.sh -s . -o $@ \
 		-v $(APPLICATION_VERSION) \
 		-f "$(APPLICATION_FILES)"
-	$(foreach prerequisite,$^,$(call execute-command,cp -RLf $(prerequisite) $@))
+	cp -RLf $< $@
+


Ooh, never realised you could include blank lines between build-steps in a Makefile before. But I've just tested on Linux and it seems to work fine - neat!

lurch · 2017-04-07T12:20:12Z

lib/shared/errors.js

- *   console.log('We should report this error');
- * }
- */
-exports.shouldReport = (error) => {


Is this handled internally in resin-corvus now?

(and does resin-corvus do all the same "anonymising" that we used to do in Etcher?)

Yeah, all that logic has been moved to resin-corvus, so we can re-use it with other resin.io products.

lurch · 2017-04-07T12:23:00Z

scripts/build/jq-inplace.sh

@@ -19,42 +19,38 @@
 set -u
 set -e

-./scripts/build/check-dependency.sh bower
+./scripts/build/check-dependency.sh jq


Haha, quite funny that Github has 'detected' this as a renamed file ;-)

lurch · 2017-04-07T12:32:59Z

Makefile

+	cp -RLf $< $@
+
+ifdef ANALYTICS_SENTRY_TOKEN
+	./scripts/build/jq-inplace.sh \


Given the limited use of jq-inplace.sh, I wonder if it'd be clearer to name it e.g. jq-insert.sh, and then pass it the property to insert (analytics.sentry.token) and value to insert ($(ANALYTICS_SENTRY_TOKEN)) as separate parameters? *shrug*

@jviotti What do you think, is it worth it?

Sounds good to me 👍

Created a new commit so you can review separately: a0f1b05

lurch · 2017-04-07T12:34:56Z

scripts/build/jq-inplace.sh

 fi

+TEMPORARY_FILE="$ARGV_TEMPORARY_DIRECTORY/$(basename $ARGV_FILE).TMP"


Should probably quote $ARGV_FILE here, i.e.

TEMPORARY_FILE="$ARGV_TEMPORARY_DIRECTORY/$(basename "$ARGV_FILE").TMP"

jviotti · 2017-04-07T14:48:18Z

@lurch

So what's the syntax used by require() to determine whether require('resin-corvus/browser') means to look for an installed module named resin-corvus, or whether to look for a local file named resin-corvus/browser.js ? Is it simply the case that all 'local requires' must start with ./ or ../ ?

If the path starts with ./ or ../, then its considered a relative require. Otherwise, resin-corvus/browser resolves to a browser.js file inside node_modules/resin-corvus.

For this case, if the require path is not relative, then we should only try to detect if resin-corvus is present.

stefan-mihaila · 2017-04-07T16:33:30Z

I guess we have to merge #1267 to the CI builds will pass

jviotti · 2017-04-09T00:01:43Z

The PR has been merged!

This commit also removes bower. Change-Type: patch Fixes: #1027

stefan-mihaila requested a review from jviotti March 23, 2017 17:29

stefan-mihaila added the in progress label Mar 23, 2017

stefan-mihaila changed the title ~~feat(GUI): use resin-raven in AnalyticsService~~ [WIP] feat(GUI): use resin-raven in AnalyticsService Mar 23, 2017

stefan-mihaila force-pushed the resin-raven branch 3 times, most recently from 8b8e175 to b265590 Compare March 23, 2017 18:45

jviotti suggested changes Mar 23, 2017

View reviewed changes

stefan-mihaila force-pushed the resin-raven branch from b265590 to c428c6f Compare March 23, 2017 23:23

stefan-mihaila force-pushed the resin-raven branch from c428c6f to dfc1626 Compare March 23, 2017 23:49

stefan-mihaila changed the title ~~[WIP] feat(GUI): use resin-raven in AnalyticsService~~ feat(GUI): use resin-raven in AnalyticsService Mar 24, 2017

stefan-mihaila force-pushed the resin-raven branch from dfc1626 to 17592de Compare March 24, 2017 17:17

stefan-mihaila changed the title ~~feat(GUI): use resin-raven in AnalyticsService~~ [WIP] feat(GUI): use resin-raven in AnalyticsService Mar 24, 2017

stefan-mihaila force-pushed the resin-raven branch 6 times, most recently from 586f231 to 09b0250 Compare March 27, 2017 20:11

stefan-mihaila changed the title ~~[WIP] feat(GUI): use resin-raven in AnalyticsService~~ [WIP] feat(GUI): use resin-corvus in AnalyticsService Mar 27, 2017

stefan-mihaila force-pushed the resin-raven branch 2 times, most recently from f0643ed to 1bca884 Compare March 27, 2017 20:55

jviotti suggested changes Mar 27, 2017

View reviewed changes

stefan-mihaila changed the title ~~[WIP] feat(GUI): use resin-corvus in AnalyticsService~~ feat(GUI): use resin-corvus in AnalyticsService Mar 31, 2017

jviotti approved these changes Mar 31, 2017

View reviewed changes

stefan-mihaila removed the in progress label Apr 3, 2017

stefan-mihaila force-pushed the resin-raven branch from b3bea9d to 11b77f9 Compare April 3, 2017 09:54

stefan-mihaila added the ready label Apr 3, 2017

stefan-mihaila force-pushed the resin-raven branch from 11b77f9 to 1feebfe Compare April 6, 2017 19:09

lurch reviewed Apr 7, 2017

View reviewed changes

stefan-mihaila force-pushed the resin-raven branch from 1feebfe to 83f2c04 Compare April 7, 2017 16:22

jviotti mentioned this pull request Apr 9, 2017

chore: fix CI script to work when 'require'ing nested modules #1267

Merged

lurch mentioned this pull request Apr 9, 2017

chore: fix a CI script to 'properly fail' on Windows #1270

Merged

stefan-mihaila force-pushed the resin-raven branch 2 times, most recently from 9f39147 to 662c90a Compare April 10, 2017 14:43

stefan-mihaila added 2 commits April 10, 2017 17:31

feat(GUI): use resin-raven in AnalyticsService

c4053c1

This commit also removes bower. Change-Type: patch Fixes: #1027

Give parameters separately to jq-insert

a0f1b05

stefan-mihaila force-pushed the resin-raven branch from 662c90a to a0f1b05 Compare April 10, 2017 16:32

jviotti merged commit 91a1c3d into master Apr 10, 2017

jviotti deleted the resin-raven branch April 10, 2017 19:06

snyk-bot mentioned this pull request Jul 12, 2021

[Snyk] Fix for 17 vulnerabilities hixio-mh/etcher#4

Open

snyk-bot mentioned this pull request Sep 7, 2021

[Snyk] Fix for 2 vulnerabilities hixio-mh/etcher#14

Open

		fi

		TEMPORARY_FILE="$ARGV_TEMPORARY_DIRECTORY/$(basename $ARGV_FILE).TMP"

feat(GUI): use resin-corvus in AnalyticsService #1208

feat(GUI): use resin-corvus in AnalyticsService #1208

Conversation

stefan-mihaila commented Mar 23, 2017 • edited Loading

jviotti commented Mar 23, 2017

jviotti commented Mar 23, 2017

jviotti commented Mar 23, 2017 • edited Loading

jviotti commented Mar 23, 2017

jviotti commented Mar 23, 2017

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

stefan-mihaila commented Mar 23, 2017

jviotti commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefan-mihaila Mar 27, 2017 • 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

stefan-mihaila commented Mar 27, 2017

stefan-mihaila commented Mar 31, 2017

jviotti commented Mar 31, 2017

stefan-mihaila commented Apr 3, 2017

jviotti commented Apr 4, 2017

stefan-mihaila commented Apr 6, 2017

jviotti commented Apr 6, 2017

jviotti commented Apr 6, 2017 • edited Loading

lurch commented Apr 7, 2017

Choose a reason for hiding this comment

stefan-mihaila Apr 7, 2017 • 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

stefan-mihaila Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jviotti commented Apr 7, 2017

stefan-mihaila commented Apr 7, 2017

jviotti commented Apr 9, 2017

feat(GUI): use `resin-corvus` in AnalyticsService #1208

feat(GUI): use `resin-corvus` in AnalyticsService #1208

stefan-mihaila commented Mar 23, 2017 •

edited

Loading

jviotti commented Mar 23, 2017 •

edited

Loading

stefan-mihaila Mar 27, 2017 •

edited

Loading

jviotti commented Apr 6, 2017 •

edited

Loading

stefan-mihaila Apr 7, 2017 •

edited

Loading

stefan-mihaila Apr 10, 2017 •

edited

Loading