-
Notifications
You must be signed in to change notification settings - Fork 86
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
Migration to new analytics #321
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! Presumably the commented out code will be addressed so we don't have dead code floating around, and then we'll make sure to snapshot or otherwise test the events coming out, so we understand if any of those change over time.
UserBeganAddingNewUrl: null, | ||
UserFinishedAddingNewUrl: null, | ||
DiffWasReset: null, | ||
PreviewSuggestion: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one doesn't fit the grammar, and it seems like we're inconsistently applying the User prefix.
|
||
//Errors | ||
JavascriptErrorDetectedInFrontend: null, | ||
RequirementForDiffsToHaveASuggestionFailed: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lou might have a suggestion for assigning these numeric codes or something to make it easier to add more, otherwise seems fine
return { | ||
type: type, | ||
context: { | ||
clientAgent: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these we'll need to figure out where they come from. if all tracking is authenticated we can fill in clientId on the server side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it stubs empty and each client sets these themselves
@@ -0,0 +1,7 @@ | |||
const { ApiCreated } = require('../src/events/onboarding'); | |||
|
|||
test('Can creeate an event type', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opportunity to fix spelling errors is now, otherwise you'll also have to edit the snapshot file
const clientContext: ClientContext = { | ||
clientAgent: clientAgent, | ||
clientId: clientId, | ||
clientSessionInstanceId: batchId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clientSessionInstanceId corresponds to the client lifecycle. For example, in a browser each tab would have a different one, and each cli run would have a different one. batchId is an additional field, like a correlation or causation id, helping to group events by their originating cause
workspaces/analytics/.prettierrc
Outdated
@@ -0,0 +1,5 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is in the workspace, it should inherit the workspace prettierrc correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it started separated. cleaning it now
@@ -0,0 +1,2 @@ | |||
import { v4 as uuidv4 } from 'uuid'; | |||
export const consistentAnonymousId = `anonymous_user_${uuidv4()}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to document here that it must start with anonymous_, or capture that in some test
RequirementForDiffsToHaveASuggestionFailed: null, | ||
|
||
//running tasks | ||
StartedTaskWithLocalCLI: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with Api, etc., these should say Cli instead of CLI
`http://localhost:${daemonState.port}/api` | ||
); | ||
|
||
cliServerClient.postTrackingEvents([event]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably fine since it's local but in the future we may wish to queue & batch flush
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request has been automatically deployed to FeaturePeek. 👏
Your deployment will be kept up-to-date with this pull request's latest changes.
Please read our docs for more configuration details.
…dev/optic into migration-to-new-analytics
}); | ||
} | ||
export async function trackUserEvent(event: TrackingEventBase<any>) { | ||
const daemonState = await ensureDaemonStarted( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devdoshi it's here
* fixed color * reordered analytics * reordered analytics * removed testing dashboard from navbar * cut excess line, also navbar link * fixed message not showing * font weight * added cache control * added border and increased size of snackbar * fixed breaking issue where wrong message was showing * updated intro message * implemented feedback for interactive demo * added snooze tooltips * removed info button and adjusted language * made sure to redirect to undoc url page when needed * updated button * button v2 * added spacing between button and box * cleaned up hiding contract testing * setup * Fully integrated demo with analytics - found one bug with get request * making new demo page * merged with analytics
This is a draft of the new approach to analytics we want to adopt. We're starting to use user events from across all the various parts of Optic (local UI, CLI, SaaS) to power demos, on-boarding flows, support interactions, etc. These are going to be key to the user experience and we don't want an extra space or a small change in the tense of an event name to break one of these experiences.
These changes introduce types for the analytics (open to recommendations to make this cleaner. it is quite hard to get TS runtime + compile time validation).
Consistent naming for analytics events
All User Tracking Events are published through Event Emitters -- segment, our backend, the demo + in-app interactions etc read off these streams.
This is in draft form. It will need to be approved before we can release on-boarding step 3 which consumes these events and persists some of them to the backend to power the on-boarding flow.
@trulyronak we need to talk about the demo's events.
@LouManglass some of the names here change