Analytics Testing & Exploration [AARD-1937]
#1237
Conversation
rutmanz
requested review from
AlexD717,
BrandonPacewic,
Dhruv-0-Arora and
PepperLola
azaleacolburn
left a comment
azaleacolburn
left a comment
There was a problem hiding this comment.
Seems like there are some security issues from some third-party app, is that something we care about.
| assemblyName: displayName, | ||
| fileSize: buffer.byteLength, | ||
| key, | ||
| type: miraType == MiraType.ROBOT ? "robot" : "field", |
There was a problem hiding this comment.
Could you put a note in #1190 to fix this if this PR gets merged first.
|
@azaleacolburn From an actual "is this a vulnerability" standpoint the answer is "no." The thing it is flagging is not a type of vulnerability that matters to us and that the library is used in the build process, not when serving. However, from a "we don't want Snyk errors" perspective, it might make sense to try and find a different way of accomplishing the git commit hash inclusion (@PepperLola @BrandonPacewic?) |
BrandonPacewic
left a comment
BrandonPacewic
left a comment
There was a problem hiding this comment.
@azaleacolburn From an actual "is this a vulnerability" standpoint the answer is "no." The thing it is flagging is not a type of vulnerability that matters to us and that the library is used in the build process, not when serving. However, from a "we don't want Snyk errors" perspective, it might make sense to try and find a different way of accomplishing the git commit hash inclusion (@PepperLola @BrandonPacewic?)
I'm honestly not 100% sure, if its not possible to mark this issue as ignored then we need to find some sort of workaround. At some point we should document all of our internal org checks as some of these things get flagged and sent to Pam.
|
did a workaround so the dependency is no longer included |
BrandonPacewic
left a comment
BrandonPacewic
left a comment
There was a problem hiding this comment.
Think this looks good, again going to have conflicts with #1241, doesn't look too bad. Will revisit merge order once we see how refactor is going.
PepperLola
left a comment
PepperLola
left a comment
There was a problem hiding this comment.
There are a couple unrelated formatting changes but I don't really mind that, otherwise looks good
Task
AARD-1937
Discover what we're doing with analytics and write unit tests
Symptom
No unit tests, analytics kind of obscure and Google Analytics is a confusing platform
Solution
I wrote tests to validate that the
AnalyticsSystem.ts=>google-analytics.compipeline was working and all the intermediate components were present and workingI went into the google analytics page and poked around until I could find where the data was going, and discovered that the user attributes we had been sending were unusable because they had spaces (oops).
I added typing to the analytics events (at least for keys) and added an event for selecting an input scheme.
In terms of processing analytics, the user attributes will give us more flexibility over what we filter, I found (kind of) how to see event info, and we can filter by developer or not.
Verification
Tests pass, removing important parts of the code makes them not pass. Analytics are sent and can be seen live in the debug page of the GA dashboard (the main pages take a while to update)
Before merging, ensure the following criteria are met: