-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
Add telemetry to frontend app #2513
Conversation
@kgodey I would like you to review the logged event name and metadata attached to the events and make sure they are sensible. |
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.
I have one comment @silentninja
logEvent('opened_schema', { | ||
database_name: database.name, | ||
schema_name: schema.name, | ||
page: 'schema', |
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.
What does page
refer to here? It's a little vague so I'd prefer a more verbose name.
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.
@kgodey page
refers to the screen/page where the event/action took place. Does the name event_location
or source_page
convey the intent better?
In case you want to understand the reason behind the page
attribute. It is useful in case an action can be performed in more than one place. For example, we can create a Table from
- The schema overview page (/<db_name>/<schema_id>/) . The event will be
opened_create_table
and the associated metadata will bepage: schema
. - The tables page (/<db_name>/<schema_id>/tables/). The event will be
opened_create_table
and the associated metadata will bepage: tables
.
Thepage
attribute is helpful if we want to analyse the preferred page to create a Table.
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.
Makes sense @silentninja. I would do something like "source": "schema_page"
since not all starting points are pages (e.g. the record selector might be a source).
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.
@silentninja This looks good. I have a couple minor comments.
Regarding the events, I know that simple analytics automatically tracks route changes. We have only one additional custom event here opened_schema
.
I assume we'd add more events to provide us better insight. Is there an issue to track the events we want?
mathesar_ui/src/utils/telemetry.ts
Outdated
metadata: Record<string, string | number | boolean | Date>, | ||
): void { | ||
window.dispatchEvent( | ||
new CustomEvent('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.
Can we change the name of this to userAction
or something similar? event
is too generic a term to use here.
@seancolsen, @rajatvijay Do you have any suggestions?
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.
userAction
makes sense.
demo/templates/demo/analytics.html
Outdated
<script> | ||
window.addEventListener("event", (e) => sa_event(e.detail.eventName, e.detail.metadata)); | ||
</script> |
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.
sa_event
would be undefined until the async analytics script loads, so the function will throw.
Event listeners ignore any errors within them, so we don't have to worry about a DOM wide error. Also, we don't dispatch any event during that time so we probably wouldn't have any lost events.
I would still like some run time checks to ensure sa_event
is defined, and e.detail
is defined, and print a console error if not, to ensure we're aware of any future issues during development. Since we can't use typescript here, I feel that a runtime check would be good. This is a pattern I'd like to discuss @seancolsen @rajatvijay
@silentninja No changes needed in this PR. If we do introduce some pattern around this, we'll take that up later.
onMount(async () => { | ||
logEvent('opened_schema', { | ||
database_name: database.name, | ||
schema_name: schema.name, |
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 does not have to be placed within onMount.
onMount
only gets called after the component is rendered to the DOM. The logEvent
function has nothing to do with the DOM, so we can directly specify it on the component.
@pavish Thanks for the review. I made the requested changes
We don't have an issue, I will one, it is on my list of things to do |
… to `userAction`
# Conflicts: # mathesar_ui/src/pages/schema/SchemaPage.svelte
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.
Looks good!
Fixes #2502
Adds functions to log user interaction events. There is a slight change in the implemented infrastructure compared to agreed infrastructure in the sync meeting. I have added the details in the
Technical Details
section belowTechnical details
We dispatch the logged user interaction events as a window event, which is then picked up by an event listener that is available in the demo mode.
Checklist
Update index.md
).master
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin