Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Connect] Emit analytic events #9873
[Connect] Emit analytic events #9873
Changes from 8 commits
93602fe
dd7a73f
1ac718d
24c6c7f
f1b428e
6844553
2920d50
f421523
4bc8880
a45ccee
47dc760
6dbee83
d694aad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 talked with @mludowise-stripe and since this is a catch-all error for capturing unexpected issues, we agreed the schema doesn't need to match between iOS and Android. I picked a schema that makes sense for Android independent of what iOS has chosen.
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.
We need to ensure that we never log any PII or sensitive data. On iOS, we explicitly don't log the error's message since we can't control its contents (ex: json deserialization errors can contain the raw json which could have user-entered data like SSN or name). If we have full control over the error message on Android, then we can log it, but maybe we can add a comment that explicitly instructs adopters to ensure no PII or sensitive data is included.
Similarly, since
error
can be an arbitrary string, maybe we should add a similar comment or change this toerror_name
orerror_code
.Is
error
meant to be enough information to uniquely identify the callsite the error is coming from?On iOS, we don't control the domain+code (it could be an iOS system error), so we use the file+line number to uniquely identify the call-site. On Android, if
error
is meant to be unique and something we can directly control, then should we make this an enum (e.g.AnalyticClientErrorCode
or something)? Okay to leave it as a string, but maybe we add a comment stating that it should be uniquely identifiable so it can be used to trace the call site in code.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 add more context: we plan to setup per-platform alerts for these these generic client errors. So adding a uniquely identifiable error_code could also be useful if we ever wanted to configure those alerts to be more fine-grained, but not a requirement.
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 we should only log the URL's host in the message, rather than the entire URL. It's possible the URL could contain sensitive data (e.g. if we get a bug in FinancialConnections integration, it could be a bank auth redirect and contain an oauth token as a query param).