PIR: Add pixels related to bundle json usage#8012
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Privacy Review task: https://app.asana.com/0/69071770703008/1213725618532951 |
|
Privacy Review task: https://app.asana.com/0/69071770703008/1213725618568468 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Missing
vpn_connection_stateparameter in failure pixel- Made reportBundleBrokerJsonFailure a suspend function and added the vpn_connection_state parameter to both the Kotlin implementation and JSON definition to match the pattern of other failure pixels.
Or push these changes by commenting:
@cursor push d7fdf105b3
Preview (d7fdf105b3)
diff --git a/PixelDefinitions/pixels/personal_information_removal.json5 b/PixelDefinitions/pixels/personal_information_removal.json5
--- a/PixelDefinitions/pixels/personal_information_removal.json5
+++ b/PixelDefinitions/pixels/personal_information_removal.json5
@@ -1061,6 +1061,11 @@
"key": "error_details",
"description": "Sanitized exception message from the bundle load failure",
"type": "string"
+ },
+ {
+ "key": "vpn_connection_state",
+ "description": "Reported VPN connection state when the pixel is fired",
+ "type": "string"
}
]
},
diff --git a/pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/pixels/PirPixelSender.kt b/pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/pixels/PirPixelSender.kt
--- a/pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/pixels/PirPixelSender.kt
+++ b/pir/pir-impl/src/main/java/com/duckduckgo/pir/impl/pixels/PirPixelSender.kt
@@ -552,7 +552,7 @@
suspend fun reportBundleBrokerJsonLoaded()
- fun reportBundleBrokerJsonFailure(
+ suspend fun reportBundleBrokerJsonFailure(
message: String,
)
@@ -1322,9 +1322,10 @@
enqueueFire(PIR_BUNDLE_BROKER_JSON_LOADED, params)
}
- override fun reportBundleBrokerJsonFailure(message: String) {
+ override suspend fun reportBundleBrokerJsonFailure(message: String) {
val params = mapOf(
PARAM_KEY_ERROR_DETAILS to message,
+ PARAM_KEY_VPN_STATE to networkProtectionState.safeIsVpnRunning().toVpnConnectionState(),
)
fire(PIR_BUNDLE_BROKER_JSON_FAILURE, params)
}| PARAM_KEY_ERROR_DETAILS to message, | ||
| ) | ||
| fire(PIR_BUNDLE_BROKER_JSON_FAILURE, params) | ||
| } |
There was a problem hiding this comment.
Missing vpn_connection_state parameter in failure pixel
Low Severity
reportBundleBrokerJsonFailure omits the vpn_connection_state parameter, unlike every other failure pixel in this file (e.g., reportDownloadBrokerJsonFailure, reportDownloadMainConfigFailure) and even unlike its companion reportBundleBrokerJsonLoaded pixel which includes it. The omission appears in both the JSON definition and implementation, making the method non-suspend, but it breaks the diagnostic pattern where VPN state is included in failure pixels to aid in root cause analysis. If VPN interference is a potential cause of bundle load failures, this data will be missing.
Please tell me if this was useful or not with a 👍 or 👎.
Additional Locations (1)
There was a problem hiding this comment.
Do we need vpn state on this new pixels?
There was a problem hiding this comment.
Not for this failure. IF the bundled json fails, then it will not be due to any network issue.
landomen
left a comment
landomen
left a comment
There was a problem hiding this comment.
Works as expected 👍
One thing I noticed is that we still emit m_dbp_update_databrokers_success_c for every broker even when loaded from json. Is that expected?
| PARAM_KEY_ERROR_DETAILS to message, | ||
| ) | ||
| fire(PIR_BUNDLE_BROKER_JSON_FAILURE, params) | ||
| } |
There was a problem hiding this comment.
Do we need vpn state on this new pixels?
2 participants
Task/Issue URL: https://app.asana.com/1/137249556945/task/1213720291861205?focus=true
Description
See attached task description
Steps to test this PR
Run https://app.asana.com/1/137249556945/task/1213721083788440?focus=true and ensure that: