[iOS] Rename empty-string variant in iOS save prompt histograms [chromium/src : main]
yiwen qian (Gerrit)
yiwen qian added 1 comment![]( "Open in Gerrit")
shown or not shown for a card to be saved as a {SaveDestination} card, and
this card is not saved with CVC. Is emitted whenever card save is offered orSlobodan PejicOh, I...wasn't aware of this. I thought the `""` was the parent aggregate metric and `.SavingWithCvc` was the subset that included CVC. That's generally how we do our metrics. If this version *without* `.SavingWithCvc` means that CVC was *explicitly missing*, then we should change this to be `.SavingWithoutCvc` instead of blank.
That might even be better, because then you don't have to do the rest of this at all because there's no longer an empty histogram variant! (Although I generally still do recommend having a parent histogram that captures *all* data...)
CC @smcg...@chromium.org, thoughts?
Olivia Sauldrive-by: IIRC the intent was to have
- 2 high-level aggregate metrics for local and server - `Autofill.SaveCreditCardPrompt.(Local|Server)` (note: the metrics dashboard can slice these per-platform as well), and
- the fine grained per platforms metrics for their specific UI treatment. `Autofill.SaveCreditCardPrompt.(IOS|Android|Desktop)....`
(go/save-card-metric-refactor)
Context: On IOS we are displaying the bottom sheet only when there are no strikes and also both the expiry nor the cardholder name are present (the bottom sheet does not support editing yet).
Stephen McGruerThanks for the added context Slobo; agreed. Let's wait for Stephen to come back in a couple weeks and see what he thinks, Yiwen.
yiwen qianSlobo is correct here. The goal is to have high-level aggregate methods that make sense across all platforms (and so can be considered in summation across platforms with the usual caveats about UMA extrapolation), then each platform can emit platform-specific metrics if it needs them.
In the case of iOS, we needed to measure + compare across pre-bottomsheet and post-bottomsheet behaviors , and needed to split out the pre-bottomsheet behaviors to specifically match the cases where we were going to offer a bottomsheet (0 strikes, no-fix-flow). So we created something very specific.
For CVC save, the question is probably what do you actually need/want to measure here? What data use-cases do you care about, what decisions are you looking to make, etc :).
Thanks everyone for the context and feedback.
Based on the discussion, I've updated the CL to rename the empty string variant to `.SavingWithoutCvc`. This avoids the issue with empty variants and makes it explicit that the metrics distinguish between saving *with* CVC and *without* CVC (rather than one being an aggregate).
Related details
- Olivia Saul
- Slobodan Pejic
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivia Saul (Gerrit)
Olivia Saul voted and added 3 comments![]( "Open in Gerrit")
Votes added by Olivia Saul
| Code-Review | +1 |
3 comments
Approving on the basis that it fixes the issue with the empty string histogram variant.
This LGTM is not a commentary on if this is the *right* thing to log or not. That's kind of separate, and ideally would have been handled in the CL before this one.
Looks like the automated metrics tracker is pointing out that a lot of metrics moved; please add an obsoletion message as it mentions, explaining that the data has moved.
<variants name="AutofillSaveCardWithCvcSituation">Please drop the leading `.` from these and move it to the two histograms that utilize it
Related details
- Slobodan Pejic
- Stephen McGruer
- yiwen qian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
yiwen qian (Gerrit)
yiwen qian added 2 comments![]( "Open in Gerrit")
Looks like the automated metrics tracker is pointing out that a lot of metrics moved; please add an obsoletion message as it mentions, explaining that the data has moved.
The code modified both variant name and histogram name, so the metrics warning persists even though the obsolete message added.
Please drop the leading `.` from these and move it to the two histograms that utilize it
- Olivia Saul
- Slobodan Pejic
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivia Saul (Gerrit)
Olivia Saul voted and added 1 comment![]( "Open in Gerrit")
Votes added by Olivia Saul
| Code-Review | +1 |
- Slobodan Pejic
- Stephen McGruer
- yiwen qian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
yiwen qian (Gerrit)
yiwen qian added 1 comment![]( "Open in Gerrit")
Hi Tommy and Sylvain, could you please review this CL? Thank you!
Related details
- Slobodan Pejic
- Stephen McGruer
- Sylvain Defresne
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Martino (Gerrit)
Tommy Martino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tommy Martino
| Code-Review | +1 |
2 comments
lgtm. However, please also get an lgtm from either Stephen or Slobodan before landing.
".Server.BottomSheet.SavingWithoutCvc"}),nit: Since you defined a constant above for this, please use it here (and lines 252, 592, 598, 743). You can simply add it as a third item in the list of strings to strcat.
Same goes for the other unit test files where you added a constant.
Related details
- Slobodan Pejic
- Stephen McGruer
- Sylvain Defresne
- yiwen qian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen McGruer (Gerrit)
Stephen McGruer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen McGruer
| Code-Review | +1 |
Approving on the basis that it fixes the issue with the empty string histogram variant.
This LGTM is not a commentary on if this is the *right* thing to log or not. That's kind of separate, and ideally would have been handled in the CL before this one.
Ack. As per the other thread, those are concerns I have (that this metric was unnecessarily appended to rather than actually figuring out what we care about measuring for CVCs), but since this doesn't actually increase the combinatorial complexity vs what already landed and it fixes the bug that the histogram started meaning a different thing once CVC save was landed, I think it's ok...
If we plan to take bottomsheet to more than just num-strikes=0 && no-fix-flow (e.g., imagine we want to take it to num-strikes=0-1) this might be a pain as we now need to sum across CVC histograms. But we'll cross that bridge when we get there (and like I said, that pain has already been landed when the CVC metrics were first landed).
Related details
- Slobodan Pejic
- Sylvain Defresne
- yiwen qian
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
yiwen qian (Gerrit)
yiwen qian voted and added 2 comments![]( "Open in Gerrit")
Votes added by yiwen qian
| Commit-Queue | +1 |
2 comments
nit: Since you defined a constant above for this, please use it here (and lines 252, 592, 598, 743). You can simply add it as a third item in the list of strings to strcat.
Same goes for the other unit test files where you added a constant.
Done
shown or not shown for a card to be saved as a {SaveDestination} card, and
this card is not saved with CVC. Is emitted whenever card save is offered orSlobodan PejicOh, I...wasn't aware of this. I thought the `""` was the parent aggregate metric and `.SavingWithCvc` was the subset that included CVC. That's generally how we do our metrics. If this version *without* `.SavingWithCvc` means that CVC was *explicitly missing*, then we should change this to be `.SavingWithoutCvc` instead of blank.
That might even be better, because then you don't have to do the rest of this at all because there's no longer an empty histogram variant! (Although I generally still do recommend having a parent histogram that captures *all* data...)
CC @smcg...@chromium.org, thoughts?
Olivia Sauldrive-by: IIRC the intent was to have
- 2 high-level aggregate metrics for local and server - `Autofill.SaveCreditCardPrompt.(Local|Server)` (note: the metrics dashboard can slice these per-platform as well), and
- the fine grained per platforms metrics for their specific UI treatment. `Autofill.SaveCreditCardPrompt.(IOS|Android|Desktop)....`
(go/save-card-metric-refactor)
Context: On IOS we are displaying the bottom sheet only when there are no strikes and also both the expiry nor the cardholder name are present (the bottom sheet does not support editing yet).
Stephen McGruerThanks for the added context Slobo; agreed. Let's wait for Stephen to come back in a couple weeks and see what he thinks, Yiwen.
yiwen qianSlobo is correct here. The goal is to have high-level aggregate methods that make sense across all platforms (and so can be considered in summation across platforms with the usual caveats about UMA extrapolation), then each platform can emit platform-specific metrics if it needs them.
In the case of iOS, we needed to measure + compare across pre-bottomsheet and post-bottomsheet behaviors , and needed to split out the pre-bottomsheet behaviors to specifically match the cases where we were going to offer a bottomsheet (0 strikes, no-fix-flow). So we created something very specific.
For CVC save, the question is probably what do you actually need/want to measure here? What data use-cases do you care about, what decisions are you looking to make, etc :).
Thanks everyone for the context and feedback.
Based on the discussion, I've updated the CL to rename the empty string variant to `.SavingWithoutCvc`. This avoids the issue with empty variants and makes it explicit that the metrics distinguish between saving *with* CVC and *without* CVC (rather than one being an aggregate).
- Olivia Saul
- Slobodan Pejic
- Stephen McGruer
- Sylvain Defresne
- Tommy Martino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |