[Pix Iframe] Log iframe transaction results [chromium/src : main]
Longsheng Yin (Gerrit)
Longsheng Yin added 1 comment![]( "Open in Gerrit")
Related details
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siddharth Shah (Gerrit)
Siddharth Shah added 1 comment![]( "Open in Gerrit")
name="FacilitatedPayments.Pix.Transaction.{Result}.Latency.{FrameType}"I'd prefer to instead create a new histogram tp avoid the packing too much information in one histogram. FacilitatedPayments.Pix.{FrameType}.TransactionResult This could have the result specified as success/failure.
In addition, I think we also need to create a new PixCodeCopied.Iframe histogram to count the total number of times, we detect the code in iframes. WDYT?
Related details
- Longsheng Yin
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. |
Longsheng Yin (Gerrit)
Longsheng Yin added 1 comment![]( "Open in Gerrit")
name="FacilitatedPayments.Pix.Transaction.{Result}.Latency.{FrameType}"I'd prefer to instead create a new histogram tp avoid the packing too much information in one histogram. FacilitatedPayments.Pix.{FrameType}.TransactionResult This could have the result specified as success/failure.
In addition, I think we also need to create a new PixCodeCopied.Iframe histogram to count the total number of times, we detect the code in iframes. WDYT?
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Longsheng Yin (Gerrit)
Longsheng Yin added 1 comment![]( "Open in Gerrit")
name="FacilitatedPayments.Pix.Transaction.{Result}.Latency.{FrameType}"Longsheng YinI'd prefer to instead create a new histogram tp avoid the packing too much information in one histogram. FacilitatedPayments.Pix.{FrameType}.TransactionResult This could have the result specified as success/failure.
In addition, I think we also need to create a new PixCodeCopied.Iframe histogram to count the total number of times, we detect the code in iframes. WDYT?
Sounds good, let me update histograms.
@sia...@google.com once iframe flag is enabled, FacilitatedPayments.Pix.InitiatePurchaseAction and FacilitatedPayments.Pix.Transaction will have data from both main frame and iframe without knowing the frame types, do we expect this? or do you think we can rename the logging methods and only log when the frame type is check, like:
```
if (is_iframe_) {
LogPixTransactionResultAndLatencyForIframe(){..}
} else {
LogPixTransactionResultAndLatencyForMainFrame(){..}
}
```
Related details
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siddharth Shah (Gerrit)
Siddharth Shah added 1 comment![]( "Open in Gerrit")
name="FacilitatedPayments.Pix.Transaction.{Result}.Latency.{FrameType}"Longsheng YinI'd prefer to instead create a new histogram tp avoid the packing too much information in one histogram. FacilitatedPayments.Pix.{FrameType}.TransactionResult This could have the result specified as success/failure.
In addition, I think we also need to create a new PixCodeCopied.Iframe histogram to count the total number of times, we detect the code in iframes. WDYT?
Longsheng YinSounds good, let me update histograms.
@sia...@google.com once iframe flag is enabled, FacilitatedPayments.Pix.InitiatePurchaseAction and FacilitatedPayments.Pix.Transaction will have data from both main frame and iframe without knowing the frame types, do we expect this? or do you think we can rename the logging methods and only log when the frame type is check, like:
```
if (is_iframe_) {
LogPixTransactionResultAndLatencyForIframe(){..}
} else {
LogPixTransactionResultAndLatencyForMainFrame(){..}
}```
I believe we still need the `FacilitatedPayments.Pix.InitiatePurchaseAction` and `FacilitatedPayments.Pix.Transaction` to log as they do today. This is because some of the existing metrics dashboards use this metric.
We can introduce a new histogram to distinguish between iframe and mainframe mainly for the launch so that we are able to measure any regressions. Post launch, I think we should be able to deprecate this metric and only rely on the combined one. WDYT?
Related details
- Longsheng Yin
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. |
Longsheng Yin (Gerrit)
Longsheng Yin added 1 comment![]( "Open in Gerrit")
name="FacilitatedPayments.Pix.Transaction.{Result}.Latency.{FrameType}"Longsheng YinI'd prefer to instead create a new histogram tp avoid the packing too much information in one histogram. FacilitatedPayments.Pix.{FrameType}.TransactionResult This could have the result specified as success/failure.
In addition, I think we also need to create a new PixCodeCopied.Iframe histogram to count the total number of times, we detect the code in iframes. WDYT?
Longsheng YinSounds good, let me update histograms.
Siddharth Shah@sia...@google.com once iframe flag is enabled, FacilitatedPayments.Pix.InitiatePurchaseAction and FacilitatedPayments.Pix.Transaction will have data from both main frame and iframe without knowing the frame types, do we expect this? or do you think we can rename the logging methods and only log when the frame type is check, like:
```
if (is_iframe_) {
LogPixTransactionResultAndLatencyForIframe(){..}
} else {
LogPixTransactionResultAndLatencyForMainFrame(){..}
}```
I believe we still need the `FacilitatedPayments.Pix.InitiatePurchaseAction` and `FacilitatedPayments.Pix.Transaction` to log as they do today. This is because some of the existing metrics dashboards use this metric.
We can introduce a new histogram to distinguish between iframe and mainframe mainly for the launch so that we are able to measure any regressions. Post launch, I think we should be able to deprecate this metric and only rely on the combined one. WDYT?
sg! updated this CL to add `FacilitatedPayments.Pix.Transaction.{FrameType}.Result`, ptal.
Related details
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siddharth Shah (Gerrit)
Siddharth Shah added 2 comments![]( "Open in Gerrit")
is_iframe_ = false;Can we also add a test to ensure that consecutive calls to OnPixCodeCopiedToClipboard resets the is_iframe_ flag correctly?
As per [here](https://source.chromium.org/chromium/chromium/src/+/main:components/facilitated_payments/core/browser/facilitated_payments_driver.cc;l=102-106;drc=6fe3560dfe2ab6b5a73655db8843a21d127336f7), if the PixManager does not get destroyed between two copy events then the `is_iframe` field would contain stale value.
TEST_P(PixManagerTestWithAccountLinkingEnabled, LogTransactionResultForIframe) {Please add a test for `LogTransactionResultForMainFrame`
Related details
- Longsheng Yin
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. |
Siddharth Shah (Gerrit)
Siddharth Shah added 1 comment![]( "Open in Gerrit")
initiate_result);Can we use [GetPurchaseActionResultString(PurchaseActionResult result)](https://source.chromium.org/chromium/chromium/src/+/main:components/facilitated_payments/core/utils/facilitated_payments_utils.h;l=12;drc=2e02d6f8818db4a9f0508d28058a9a96322db703) to format the result here instead of creating a new enum?
Longsheng Yin (Gerrit)
Longsheng Yin added 4 comments![]( "Open in Gerrit")
is_iframe_ = false;Can we also add a test to ensure that consecutive calls to OnPixCodeCopiedToClipboard resets the is_iframe_ flag correctly?
As per [here](https://source.chromium.org/chromium/chromium/src/+/main:components/facilitated_payments/core/browser/facilitated_payments_driver.cc;l=102-106;drc=6fe3560dfe2ab6b5a73655db8843a21d127336f7), if the PixManager does not get destroyed between two copy events then the `is_iframe` field would contain stale value.
Updated the is_iframe_ state at the beginning of OnPixCodeCopiedToClipboard and added `IsIframeStateUpdatedOnConsecutiveCalls` test, PTAL.
I am wondering will this situation happen in real world? If user clicks twice the copy button within iframe, the is_iframe_ state should be unchanged, same for main frame. Only if user clicks the copy in iframe and dismiss the selector button sheet, then clicks the copy in mainframe, the state should be reset, but in this way, there should be a copy button in both iframe and main frame, could this be possible? not sure I am understanding correctly.
TEST_P(PixManagerTestWithAccountLinkingEnabled, LogTransactionResultForIframe) {Please add a test for `LogTransactionResultForMainFrame`
Done
Can we use [GetPurchaseActionResultString(PurchaseActionResult result)](https://source.chromium.org/chromium/chromium/src/+/main:components/facilitated_payments/core/utils/facilitated_payments_utils.h;l=12;drc=2e02d6f8818db4a9f0508d28058a9a96322db703) to format the result here instead of creating a new enum?
- Siddharth Shah
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. |
Siddharth Shah (Gerrit)
Siddharth Shah added 3 comments![]( "Open in Gerrit")
is_iframe_ = false;Longsheng YinCan we also add a test to ensure that consecutive calls to OnPixCodeCopiedToClipboard resets the is_iframe_ flag correctly?
As per [here](https://source.chromium.org/chromium/chromium/src/+/main:components/facilitated_payments/core/browser/facilitated_payments_driver.cc;l=102-106;drc=6fe3560dfe2ab6b5a73655db8843a21d127336f7), if the PixManager does not get destroyed between two copy events then the `is_iframe` field would contain stale value.
Updated the is_iframe_ state at the beginning of OnPixCodeCopiedToClipboard and added `IsIframeStateUpdatedOnConsecutiveCalls` test, PTAL.
I am wondering will this situation happen in real world? If user clicks twice the copy button within iframe, the is_iframe_ state should be unchanged, same for main frame. Only if user clicks the copy in iframe and dismiss the selector button sheet, then clicks the copy in mainframe, the state should be reset, but in this way, there should be a copy button in both iframe and main frame, could this be possible? not sure I am understanding correctly.
In practice, I don't think both iframe and mainframe will show a copy button but it is good to handle it, just in case. Thanks for making the change and adding the test.
if (iframe_url.has_value()) {Can we also just use `is_iframe_` here?
is_iframe_ = true;Do we need this anymore?
Related details
- Longsheng Yin
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. |
Longsheng Yin (Gerrit)
Longsheng Yin added 2 comments![]( "Open in Gerrit")
Can we also just use `is_iframe_` here?
Done
Do we need this anymore?
- Siddharth Shah
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Siddharth Shah (Gerrit)
Siddharth Shah voted Code-Review+1![]( "Open in Gerrit")
- Longsheng Yin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Longsheng Yin (Gerrit)
Longsheng Yin added 1 comment![]( "Open in Gerrit")
Hi Olivia, could you taking a look on this change when you get a chance? Thanks.
Related details
- Olivia Saul
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Olivia Saul (Gerrit)
Olivia Saul added 5 comments![]( "Open in Gerrit")
bool is_iframe_ = false;nit: `is_iframe_` is a little vague because it's not 100% clear which "it" that "is" is referring to. What do you think of `triggered_from_iframe_`, does that work in this context?
base::StrCat({"FacilitatedPayments.Pix.Transaction.",
GetPurchaseActionResultString(result), ".Latency"}),It's not related to your CL, but I see this was added by Vishwas in crrev.com/c/6279209, but wasn't added to the spreadsheet. Would you mind doing that if you get a chance?
base::StrCat({"FacilitatedPayments.Pix.Transaction",
is_iframe ? ".Iframe" : ".MainFrame", ".",
GetPurchaseActionResultString(result)}),I expanded this entry in go/payments-autofill-uma to cover all six cases ([link](https://docs.google.com/spreadsheets/d/1R5m1KfGLRe3YBvMnjeJ_xeiDQvzTpgx4T0tZioWoqPE/edit?resourcekey=0-TuqaTwhINXPX1vhm5IQ2zA&gid=0#gid=0&range=A276)) so it's easy for people to search for and find. Can you please confirm that I did it correctly?
base::StrCat({"FacilitatedPayments.Pix.Transaction.",
GetPurchaseActionResultString(result), ".Latency"}),This appears to be removing existing test coverage for the Latency histogram? If so, please revert! Your code should be a separate test.
the frame that initiated the transaction. The frame type is {FrameType} and
result is {Result}. [Frequency] Logged at most once per Pix payflow. PixThese words in brackets get replaced with your descriptions from above. So for instance, on the UMA dashboard, this would end up saying:
*The frame type is The transaction was initiated in an iframe. and result is Abandoned InitiatePurchaseAction call.*
Please reword in such a way that this looks more grammatically correct when the UMA dashboard automatically expands the params, instead of becoming a sentence inside a sentence. Thank you!
Related details
- Longsheng Yin
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. |
Longsheng Yin (Gerrit)
Longsheng Yin added 6 comments![]( "Open in Gerrit")
Thanks! Sorry for the delay in responding, I cannot access this branch while working from China.
bool is_iframe_ = false;nit: `is_iframe_` is a little vague because it's not 100% clear which "it" that "is" is referring to. What do you think of `triggered_from_iframe_`, does that work in this context?
When `is_iframe_` is true, iframe flow will be taken and PSP URL will be checked, if the URL is not allowlisted, PIX payment won't be triggered from iframe, so I think `triggered_from_iframe_` is kind of incorrect since the triggering is not 100% guaranteed? Do you think something like `is_iframe_available_` is better?
base::StrCat({"FacilitatedPayments.Pix.Transaction.",
GetPurchaseActionResultString(result), ".Latency"}),It's not related to your CL, but I see this was added by Vishwas in crrev.com/c/6279209, but wasn't added to the spreadsheet. Would you mind doing that if you get a chance?
Sure, added to the spreadsheet.
base::StrCat({"FacilitatedPayments.Pix.Transaction",
is_iframe ? ".Iframe" : ".MainFrame", ".",
GetPurchaseActionResultString(result)}),I expanded this entry in go/payments-autofill-uma to cover all six cases ([link](https://docs.google.com/spreadsheets/d/1R5m1KfGLRe3YBvMnjeJ_xeiDQvzTpgx4T0tZioWoqPE/edit?resourcekey=0-TuqaTwhINXPX1vhm5IQ2zA&gid=0#gid=0&range=A276)) so it's easy for people to search for and find. Can you please confirm that I did it correctly?
That's correct, thank you!
base::StrCat({"FacilitatedPayments.Pix.Transaction.",
GetPurchaseActionResultString(result), ".Latency"}),This appears to be removing existing test coverage for the Latency histogram? If so, please revert! Your code should be a separate test.
Sorry, added it back.
the frame that initiated the transaction. The frame type is {FrameType} and
result is {Result}. [Frequency] Logged at most once per Pix payflow. PixThese words in brackets get replaced with your descriptions from above. So for instance, on the UMA dashboard, this would end up saying:
*The frame type is The transaction was initiated in an iframe. and result is Abandoned InitiatePurchaseAction call.*
Please reword in such a way that this looks more grammatically correct when the UMA dashboard automatically expands the params, instead of becoming a sentence inside a sentence. Thank you!
Thanks for the detailed explanation! I now understand how to interpret the bracket notation in the summary. Reworded the summaries of this histogram, "PixFrameType" and "PurchaseActionResult". Since "PurchaseActionResult" is a shared variant, will this rewording break existing histograms?
Related details
- Olivia Saul
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. |