-
Notifications
You must be signed in to change notification settings - Fork 433
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(safe-apps): Analytics refactor #1042
Conversation
…at/safe-apps-analysis
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Deploying with Cloudflare Pages
|
</main> | ||
<> | ||
<Head> | ||
<title>Safe Apps – Share</title> |
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.
Perhaps we can include the app name in the title as a future enhancement 🤔
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.
yep, we sent the app name or url (if custom) here
if (!isLoading && safeApp?.chainIds.length) { | ||
const appName = backendApp ? backendApp.name : safeApp.url | ||
|
||
trackSafeAppEvent(SAFE_APPS_EVENTS.SHARED_APP_LANDING, appName) |
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.
Do we really need 2 separate events for this? Can the chainid be included in the SHARED_APP_LANDING
?
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.
yep, changed it. Does not make sense
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2022 Safe Web Core Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
What it solves
This PR add a refactor to the Safe Apps analytics current behavior as part of the analytics epic
How this PR fixes it
We are making changes to reflect the status described here
How to test it
We need to validate all the event actions described here are being sent
@JagoFigueroa the preference is validate this using GTM and GA4 tools. We can align on this
Analytics changes
We now are sending all the analytics from Safe Apps using the concrete
safeApp
event or the custompageview
. Previously it was a combination of Safe App events and Safe Web Custom events withmetadata
category. Is easier and less prone to errors to use only the Safe App event. We now have under thesafeApp
event two categories (safe-apps
andsafe-apps-sdk
)We now are sending the app name always in the
safeAppName
diimension. If the app belongs to the default list we send the name and if not the url (custom app). Previously the dimension was only being used for SDK events and we were sending the names in theeventLabel
with no clear consensus. Doing this, we know clearly if the app is known and we free a dimension (eventLabel
) for include any other dataWe send
pageview
events for the Safe Apps list and for the concrete app being loaded in order to generate reports as the time elapsed in Safe apps sections or apps