First telemetry API draft #187

delapuente · 2018-03-02T12:55:13Z

The PR adds some folder structure to the template and proposes an API for telemetry, Roughly, the telemetry module installs itself inside the UnityLoader object at UnityLoader.WebVR.telemtry. This way, there is only one entry point for everything.

To configure and start telemetry, the developer calls telemetry.start(config) and passes a config object with two properties so far granting permissions for Mozilla Research to log errors and gather analytics:

MozillaResearch.telemetry.start({
  analytics: true,
  errorLogging: true        
});

If the developer wants to add their custom tracker, they should use MozillaResearch.telemetry.ga.create() with the same signature as the create command of GA. The library then returns a function that can be treated like the regular ga() function. The pattern is inspired by @cvan proposal but provides more flexibility for the end user.

cvan

looking good, thanks for doing this. few architectural questions.

and per my dailies, I plan to revisit this by EOW. but if we want analytics ASAP this will probably suffice.

cvan · 2018-03-02T20:45:02Z

Assets/WebGLTemplates/WebVR/index.html

    <script src="%UNITY_WEBGL_LOADER_URL%"></script>
+    <script src="src/telemetry.js"></script>


this feels more like lib/

any Telemetry code should happen before the Unity Engine is loaded. it's quite possible that there are errors that get thrown which could affect the loading of that and/or this script.

either way, the code should be run before the Unity JS. so we can get accurate perf timings, catch network errors, runtime errors, etc. either it should be a blocking <script> above or an inline <script>.

cvan · 2018-03-02T20:49:01Z

Assets/WebGLTemplates/WebVR/index.html

@@ -76,8 +83,8 @@ <h3>You&rsquo;ll need a <a href="https://webvr.rocks/">WebVR-enabled browser</a>
      <button id="entervr" value="Enter VR"></button>
    </div>

-    <script src="gl-matrix-min.js"></script>
-    <script src="webvr-polyfill.min.js"></script>
+    <script src="lib/gl-matrix-min.js"></script>


this should be called vendor/

cvan · 2018-03-02T20:49:47Z

Assets/WebGLTemplates/WebVR/src/telemetry.js

+  return;
+}
+
+var navigator = window.navigator;


this is unnecessary; just use navigator directly

If we plan to test in the future, this is more convenient, don't you think?

cvan · 2018-03-02T20:50:04Z

Assets/WebGLTemplates/WebVR/src/telemetry.js

+'use strict';
+
+if (!('UnityLoader' in window)) {
+  console.warn('`UnityLoader` object not found.');


not found -> not be found

cvan · 2018-03-02T20:50:35Z

Assets/WebGLTemplates/WebVR/src/telemetry.js

+setupAnalytics();
+
+function setupAnalytics() {
+  window.ga=window.ga||function(){(ga.q=ga.q||[]).push(arguments)};ga.l=+new Date;


I know this is copied from GA's snippet builder, but might as well just unminify this

cvan · 2018-03-02T20:52:15Z

Assets/WebGLTemplates/WebVR/src/telemetry.js

+  return script;
+}
+
+function getModule (modulePath) {


maybe I'm missing the purpose: why we're using this as opposed to just calling the object directly?

cvan · 2018-03-02T20:53:02Z

Assets/WebGLTemplates/WebVR/index.html

    <script src="%UNITY_WEBGL_LOADER_URL%"></script>
+    <script src="src/telemetry.js"></script>
+    <script>
+      UnityLoader.WebVR.telemetry.start({


I like the invocation and naming of this. good work! 👍

although I'm not keen on this being attached to the UnityLoader for the aforementioned reasons.

cvan

thank you! looks good - let's get this merged but not released/deployed yet without the opt-out text instructions (per filed issues)

CSS changes may be better made in a separate PR

cvan · 2018-03-06T14:57:11Z

Assets/WebGLTemplates/WebVR/lib/telemetry.js

+}
+
+if (!('telemetry' in window.MozillaResearch)) {
+  window.MozillaResearch.telemetry = {} ;


trailing whitespace

delapuente · 2018-03-06T15:36:52Z

Makes sense. I'm going to undo the changes to CSS, remove the trailing whitespace and get it merged.

…-ga" This reverts commit cc6f8a4, reversing changes made to 285ab2f.

delapuente requested a review from cvan March 2, 2018 12:55

delapuente added this to the Telemetry milestone Mar 2, 2018

delapuente changed the title ~~Issue 169 add opt out ga~~ First telemetry API draft Mar 2, 2018

delapuente added the work in progress label Mar 2, 2018

cvan suggested changes Mar 2, 2018

View reviewed changes

delapuente mentioned this pull request Mar 6, 2018

add base Telemetry JavaScript snippet (fixes issue #172) #174

Closed

delapuente added 3 commits March 6, 2018 09:23

Add some folder structure to the project

2249e7b

First draft of telemetry interface

53993c4

New namespace for telemetry.

89a3277

delapuente removed the work in progress label Mar 6, 2018

cvan approved these changes Mar 6, 2018

View reviewed changes

Simplifying options and deminifying GA setup

4872a73

delapuente merged commit cc6f8a4 into MozillaReality:master Mar 6, 2018

delapuente added a commit that referenced this pull request Mar 6, 2018

Revert "Merge pull request #187 from delapuente/issue-169-add-opt-out…

9734410

…-ga" This reverts commit cc6f8a4, reversing changes made to 285ab2f.

cvan assigned delapuente Mar 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

First telemetry API draft #187

First telemetry API draft #187

Uh oh!

delapuente commented Mar 2, 2018 •

edited

Loading

Uh oh!

cvan left a comment

Uh oh!

cvan Mar 2, 2018

Uh oh!

cvan Mar 2, 2018

Uh oh!

cvan Mar 2, 2018

Uh oh!

cvan Mar 2, 2018

Uh oh!

delapuente Mar 5, 2018 •

edited

Loading

Uh oh!

cvan Mar 2, 2018

Uh oh!

cvan Mar 2, 2018

Uh oh!

cvan Mar 2, 2018

Uh oh!

cvan Mar 2, 2018

Uh oh!

cvan left a comment

Uh oh!

cvan Mar 6, 2018

Uh oh!

delapuente commented Mar 6, 2018

Uh oh!

Uh oh!

		<script src="%UNITY_WEBGL_LOADER_URL%"></script>
		<script src="src/telemetry.js"></script>

First telemetry API draft #187

First telemetry API draft #187

Uh oh!

Conversation

delapuente commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cvan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delapuente Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

delapuente commented Mar 6, 2018

Uh oh!

Uh oh!

delapuente commented Mar 2, 2018 •

edited

Loading

delapuente Mar 5, 2018 •

edited

Loading