feat(INJI-622): add telemetry event to track face model init fail/sucess #1101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Alka1703
suggested changes
Dec 19, 2023
shared/commonprops/commonProps.ts
Outdated
| sendErrorEvent( | ||
| getErrorEventData( | ||
| TelemetryConstants.FlowType.faceModelInit, | ||
| TelemetryConstants.EndEventStatus.failure, |
There was a problem hiding this comment.
This should be an error Id coming from Telemetry constants
shared/commonprops/commonProps.ts
Outdated
| sendErrorEvent( | ||
| getErrorEventData( | ||
| TelemetryConstants.FlowType.faceModelInit, | ||
| TelemetryConstants.EndEventStatus.failure, |
There was a problem hiding this comment.
This should the landing screen's name instead of the status. If you want to pass the status send it in additional parameters
shared/commonprops/commonProps.ts
Outdated
| @@ -21,11 +29,29 @@ export async function downloadModel() { | |||
| var result = await init(resp + '/model.tflite', false); | |||
There was a problem hiding this comment.
This is inside a for loop which means we will be logging telemetry events multiple times for the same thing.
There was a problem hiding this comment.
We should log once after retries count is exhausted
…cess Signed-off-by: Harsh Vardhan <harsh59v@gmail.com>
9befabc to
7b480ea
Compare
Signed-off-by: Harsh Vardhan <harsh59v@gmail.com>
7b480ea to
254c5d8
Compare
swatigoel
reviewed
Dec 21, 2023
| @@ -16,6 +23,7 @@ export default async function getAllConfigurations( | |||
|
|
|||
| export async function downloadModel() { | |||
| try { | |||
| console.log('restart Face model init'); | |||
There was a problem hiding this comment.
Why the log message is restart?
There was a problem hiding this comment.
Should have been removed IMO.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
(Q) What we know so far?
(A) The crash is possible on Android & iOS if the Face SDK is not initialized, there could be other reasons as well hence we are going to log an event in Telemetry and observe if a crash still exists.
(Q) Have we verified that the app crashes?
(A) Yes, I have disabled the download of face model and tried to do a Share with Selfie and a Online QR Login on Android & iOS and in both platforms the phones have crashed.
(Q) Could there be other reasons for the crash?
(A) Yes, I’ve observed one of the issue and tried to fix it and I’ll be adding telemetry events to see if the crash still happens when the face SDK model is initialized correctly.