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

feat(app): Add priority 2 analytics events #1627

Merged
merged 3 commits into from
Jun 7, 2018
Merged

feat(app): Add priority 2 analytics events #1627

merged 3 commits into from
Jun 7, 2018

Conversation

Kadee80
Copy link
Contributor

@Kadee80 Kadee80 commented Jun 5, 2018

overview

This PR closes #1553 by adding level 2 priority tracking events and tests to the app.

changelog

  • add protocolUpload, runPause, runError, runCancel events and tests

review requests

  • Get Mixpanel user ID from debug logs or config file, or Alfie. replace correct_mixpanel_id in following command to run the app.
  • make dev OT_APP_MIXPANEL_ID=correct_mixpanel_id OT_APP_ANALYTICS__OPTED_IN=1
  • watch console for logged events!

Upload Event:

Upload a protocol without errors (upload successful)

  • protocolUpload event includes {success:true, error:''}
    Upload a protocol errors (upload errors)
  • protocolUpload event includes {success:false, error: 'some error message'}

Run Events:

  • runPause event when protocol run is paused
  • runCanceled event when protocol run is canceled
  • runError event when protocol run errors (not sure how to produce this) returns {error: 'message text'}

@Kadee80 Kadee80 requested review from umbhau, mcous and IanLondon June 5, 2018 20:43
@codecov
Copy link

codecov bot commented Jun 5, 2018

Codecov Report

Merging #1627 into edge will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##            edge    #1627      +/-   ##
=========================================
+ Coverage   34.4%   34.46%   +0.06%     
=========================================
  Files        338      338              
  Lines       5526     5530       +4     
=========================================
+ Hits        1901     1906       +5     
+ Misses      3625     3624       -1
Impacted Files Coverage Δ
app/src/analytics/make-event.js 84.21% <100%> (+10.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 886f2e9...3e759ae. Read the comment docs.

Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed the following changes in real life

}
}
// $FlowFixMe(ka, 2018-06-5): flow type robot:PAUSE
case 'robot:PAUSE':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda want to trigger this on PAUSE_RESPONSE rather than PAUSE. If we do it on PAUSE_RESPONSE, then I think its properties would also include success and error like the protocolUpload event

return {name: 'runPause', properties: {}}

// $FlowFixMe(ka, 2018-06-5): flow type robot:CANCEL
case 'robot:CANCEL':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same note about CANCEL vs CANCEL_RESPONSE

case 'robot:SESSION_RESPONSE':
return {
name: 'protocolUpload',
properties: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a TODO to add file open type (upload button vs drag-n-drop) to this event?

Will likely involve attaching this information to the meta field of the SESSION_RESPONSE action itself somehow. Will get a little hairy...

@@ -45,8 +55,30 @@ export default function makeEvent (state: State, action: Action): ?Event {
const runTime = robotSelectors.getRunSeconds(state)
return {name: 'runFinish', properties: {runTime}}
} else {
// TODO(mc, 2018-05-28): runError event
return null
return {name: 'runError',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to nitpick, but can you throw in a newline so the { isn't on the same line as name? Also line 66 and line 76

@Kadee80 Kadee80 force-pushed the app_analytics-p-2 branch from 2dd801c to 9b653e3 Compare June 6, 2018 19:38
Copy link
Contributor

@mcous mcous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏓

Use responses instead of requests
@Kadee80 Kadee80 force-pushed the app_analytics-p-2 branch from 9b653e3 to fc5e8f3 Compare June 6, 2018 19:54
@Kadee80 Kadee80 merged commit 08e622e into edge Jun 7, 2018
@Kadee80 Kadee80 deleted the app_analytics-p-2 branch June 7, 2018 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User Analytics: Add Events (Priority II Events)
2 participants