-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add vpn-enable wide event #7092
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
Conversation
322aa8d to
54d5f0a
Compare
|
Privacy Review task: https://app.asana.com/0/69071770703008/1211907027107113 |
| "description": "The name of the app sending the pixel", | ||
| "enum": ["DuckDuckGo Android"] | ||
| }, | ||
| "widePixelAppVersion": { |
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 is the difference between widePixelAppVersion and appVersion? Why do we need a different param? Is it mainly to align with other platforms?
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.
Ah good question @karlenDimla . @ladamski and/or @nshuba is there more to widePixelppVersion than aligning across platforms?
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 think the wide event schema was designed without taking existing pixel params into account, because the goal is to send wide events using dedicated POST endpoint and not as a pixel. I don't think we need widePixelAppVersion now, but it will make the transition from pixel to the dedicated endpoint smoother. widePixelAppVersion is also used in existing wide events that predate pixel registry integration, and changing that here would complicate things.
...ection/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/pixels/VpnEnableWideEvent.kt
Show resolved
Hide resolved
karlenDimla
left a comment
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.
Aligns with definition and works as expected. It is worth noting (already mentioned in description) that sometimes the wide event pixel fails to send when the pixel is attempted to be sent when the VPN is not yet fully up and running - there might be some delays after startVPN has been executed. It is best to enqueue this pixel to be 100% sure that they are sent.
| }, | ||
| { | ||
| "key": "feature.data.ext.service_start_duration_ms_bucketed", | ||
| "description": "The duration of TrackerBlockingVpnService (re)start in milliseconds, bucketed", |
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 believe our default is to specify the lower end of each range. Is this lower or upper? Worth clarifying in the comment. https://app.asana.com/1/137249556945/project/1207561877052131/task/1210817094107521?focus=true
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 the upper end of each range. This bucketing logic is shared with existing wide events and AFAIK is the same on other platforms. I'm happy to change this to use lower end (TIL this should be the default), but I believe this change should be handled in a separate task.
| { | ||
| "key": "feature.data.ext.step.show_vpn_conflict_dialog", | ||
| "description": "true if the user was asked if they want to disable existing 3rd party VPN", | ||
| "enum": ["true"] |
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 it be set to false as well or only sent if true?
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.
Same question for the next 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.
It is only sent when true, I'll clarify that in the description.
| }, | ||
| "first_daily_count_short": { | ||
| "type": "string", | ||
| "description": "Pixel schedule 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.
To be super clear could say in description u= first, d=daily, c=count
This ensures that when wide event is added in vpn process, the sending logic that lives in main process will be triggered.
6b70739 to
82130c6
Compare
82130c6 to
93d0f6e
Compare

Task/Issue URL: https://app.asana.com/1/137249556945/project/1205648422731273/task/1211892474020113?focus=true
Description
This PR adds a new wide event to monitor the success rate of enabling vpn.
Steps to test this PR
Prerequisites for each test scenario
Happy path
User rejects VPN permission
Another VPN is already running
Device is offline
Enable VPN using the tile in system tray
Re-enable VPN using system notification
App tracking protection is enabled
No UI changes