[HOLD for payment 2022-11-16] [$500] Dev - Activating metrics on crashes the app on Android - reported by @parasharrajat

[mvtglobally]

trafficstars

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1.Set the env with following. ONYX_METRICS=true CAPTURE_METRICS=true 2. Build the app for android.

Expected Result:

App should build

Actual Result:

App crashes

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• Android

Version Number: Reproducible in staging?: need repro Reproducible in production?: need repro Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

Expensify/Expensify Issue URL: Issue reported by: @parasharrajat Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1660151993852989

View all open jobs on GitHub

[melvin-bot[bot]]

Triggered auto assignment to @puneetlath (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[melvin-bot[bot]]

Current assignee @puneetlath is eligible for the External assigner, not assigning anyone new.

[puneetlath]

External Upwork post: https://www.upwork.com/jobs/~01b6342fd8e2b2240a Internal Upwork job: https://www.upwork.com/ab/applicants/1564180886378299392/job-details

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (Exported)

[melvin-bot[bot]]

Current assignee @puneetlath is eligible for the Exported assigner, not assigning anyone new.

[wildan-m]

Is it OK to have originalPromise value undefined?

In node_modules/react-native-onyx/lib/metrics/index.native.js

the originalPromise is undefined when the args value is:

["isLoadingReportData", true]

If originalPromise might be undefined, then my proposal is to add undefined check

+        if(originalPromise)
            originalPromise
                .then((result) => {
                    measureMarkToNow(mark, {result});
                })
                .catch((error) => {
                    measureMarkToNow(mark, {error});
                });

[christianwen]

Proposal

Problem

For the issue in Android, it's due to the code in react-native-onyx -> lib/metrics/index.native.js -> function decorateWithMetrics is expecting that the func passed in is a function that returns a promise, which it's not always the case, for example the lib/Onyx.js -> function update(data) { does not return anything. So the attempt to use the decorateWithMetrics with that update function will result in that error because in decorateWithMetrics we're using .then with an undefined variable.

Solution

In function decorateWithMetrics, we should check that the returned value of the function is a promise first before attempting to resolve that promise. If we only check if the originalPromise is truthy like in the above proposal, it will still crash if the originalPromise returns a value that is not a promise (thus does not have the .then method). I suggest that the code can be changed to below:

+function isPromise(value){
+    return typeof value?.then === 'function';
+}

/**
 * Wraps a function with metrics capturing logic
 * @param {function} func
 * @param {String} [alias]
 * @returns {function} The wrapped function
 */
function decorateWithMetrics(func, alias = func.name) {
    if (decoratedAliases.has(alias)) {
        throw new Error(`"${alias}" is already decorated`);
    }

    decoratedAliases.add(alias);

    function decorated(...args) {
        const mark = addMark(alias, args);

+       const returnedValue = func.apply(this, args);

+       if (isPromise(returnedValue)) {
            /*
            * Then handlers added here are not affecting the original promise
            * They create a separate chain that's not exposed (returned) to the original caller
            * */
+           returnedValue
                .then((result) => {
                    measureMarkToNow(mark, {result});
                })
                .catch((error) => {
                    measureMarkToNow(mark, {error});
                });
+       }

+       return returnedValue;
    }

    return decorated;
}

The way I check if a value is a promise is also how graphql-js does it, you can refer here https://github.com/graphql/graphql-js/blob/main/src/jsutils/isPromise.ts

Android after the fix, no more crashes

About the crash in web

I'm not sure if the web crash is in the scope of this issue, but for the issue in web, it's due to the decorateWithMetrics noop in react-native-onyx -> lib/metrics/index.web.js is not returning anything, thus when we're using getAllKeys = decorate.decorateWithMetrics(getAllKeys, 'Onyx:getAllKeys');, the getAllKeys becomes undefined. So to fix that we need to modify the noop so that it returns the original function that is passed in

-function decorateWithMetrics() {}
+function decorateWithMetrics(func) { return func }

No more crash in web after the fix

Please let me know if you have any concerns regarding the above proposal Thanks!

[melvin-bot[bot]]

@puneetlath, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[Santhosh-Sellavel]

Will review this today/tomorrow.

[Santhosh-Sellavel]

@puneetlath Actually, @wildan-m suggestion solves the crash that occurs on android devices. I do prefer their solution to address the same.

From @christianwen proposal,

I'm not sure if the web crash is in the scope of this issue, but for the issue in web, it's due to the decorateWithMetrics noop in react-native-onyx -> lib/metrics/index.web.js is not returning anything, thus when we're using getAllKeys = decorate.decorateWithMetrics(getAllKeys, 'Onyx:getAllKeys');, the getAllKeys becomes undefined. So to fix that we need to modify the noop so that it returns the original function that is passed in

There is a different issue that occurs on the web, I can confirm that. We need a proposal to address that issue, but their solution doesn't address the issue.

TLD

So we are looking proposal to address the crash that occurs on the web also.

[christianwen]

Hi, @Santhosh-Sellavel in my proposal I've also put the solution to address the crash on the web by fixing the decorateWithMetrics noop. By their solution doesn't address the issue, do you mean the crash still happens for you after trying the solution?

[Santhosh-Sellavel]

Yes crash occurred for me, even after applying your suggestion,

[christianwen]

@Santhosh-Sellavel can you help attach the log of the crash after you apply the suggestion is it the same log as in the original description? thanks

[Santhosh-Sellavel]

Yeah getAllKeys was the issue, I checked it on the Latest main earlier.

[christianwen]

@Santhosh-Sellavel confirming with you, you are modifying it in the node_modules/react-native-onyx/lib/metrics/index.web.js after cloning repo https://github.com/Expensify/App locally to try right?

if that's the case then it's not working because in node_modules, it is using the built code (not the source code) of react-native-onyx in node_modules/react-native-onyx/dist/web.development.js instead. You can try modifying the built code to test as follow (line 1786)

later after the suggested change in node_modules/react-native-onyx/lib/metrics/index.web.js is merged into react-native-onyx it will work after updating the react-native-onyx version

[wildan-m]

@Santhosh-Sellavel Forgot to investigate the web version My updated proposal For android:

+         originalPromise && originalPromise
                .then((result) => {
                    measureMarkToNow(mark, {result});
                })
                .catch((error) => {
                    measureMarkToNow(mark, {error});
                });

For web:

function applyDecorators() {
    // We're requiring the script dynamically here so that it's only evaluated when decorators are used
    const decorate = require('./metrics');

    // Re-assign with decorated functions
    /* eslint-disable no-func-assign */
+    get = decorate.decorateWithMetrics(get, 'Onyx:get') || get;
+   set = decorate.decorateWithMetrics(set, 'Onyx:set') || set;
+    multiSet = decorate.decorateWithMetrics(multiSet, 'Onyx:multiSet') || multiSet;
+   clear = decorate.decorateWithMetrics(clear, 'Onyx:clear') || clear;
+   merge = decorate.decorateWithMetrics(merge, 'Onyx:merge') || merge;
+   mergeCollection = decorate.decorateWithMetrics(mergeCollection, 'Onyx:mergeCollection') || mergeCollection;
+   getAllKeys = decorate.decorateWithMetrics(getAllKeys, 'Onyx:getAllKeys') || getAllKeys;
+   initializeWithDefaultKeyStates = decorate.decorateWithMetrics(initializeWithDefaultKeyStates, 'Onyx:defaults') || initializeWithDefaultKeyStates;
+    update = decorate.decorateWithMetrics(update, 'Onyx:update') || update;
    /* eslint-enable */

The @christianwen wen solution for web version might be cleaner and not repeated

[Santhosh-Sellavel]

@Santhosh-Sellavel confirming with you, you are modifying it in the node_modules/react-native-onyx/lib/metrics/index.web.js after cloning repo https://github.com/Expensify/App locally to try right?

Ya right!

if that's the case then it's not working because in node_modules, it is using the built code (not the source code) of react-native-onyx in node_modules/react-native-onyx/dist/web.development.js instead. You can try modifying the built code to test as follow (line 1786)

This solution solves the crash on the web, need to ensure all platforms are working well before moving forward!

[melvin-bot[bot]]

@puneetlath, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Santhosh-Sellavel]

Not overdue.

Need to check on other platforms too!

[Santhosh-Sellavel]

@christianwen have you checked other platforms, does a crash occur there?

[christianwen]

@Santhosh-Sellavel yeah I've checked, no more crash on any platform, screenshots attached below

iOS

mWeb

Mac Desktop app

For Android and web I've already attached screenshots in https://github.com/Expensify/App/issues/10622#issuecomment-1234389410

Thanks!

[melvin-bot[bot]]

@puneetlath, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[puneetlath]

Updated to $500. @Santhosh-Sellavel mind providing an update?

[Santhosh-Sellavel]

Sorry for the delay here! @puneetlath Actually, @christianwen proposal solves the issue, but I am not sure this is the right way and am afraid of missing something else in the context here. So can we have a look from internal who is familiar & involved in implementing this moving forward?

[melvin-bot[bot]]

@puneetlath, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[parasharrajat]

@Santhosh-Sellavel @christianwen proposal looks good to me if we want to go with that path.

IMO, the correct fix will be https://github.com/Expensify/react-native-onyx/pull/133/files#r975658158.

Other than this,

• it requires fixing the signature of noop methods in metrics/index.web.js.
• Add/fix tests for the changes. (no proposal is valid without unit tests).

cc: @luacmartins Since you are the author of Onyx changes.

[luacmartins]

Commented on the other thread!

[puneetlath]

@parasharrajat @luacmartins just to make sure I understand, you are both saying that this Onyx PR should solve this issue. And no further action should be needed, is that correct?

[luacmartins]

Well that PR was merged a while ago, so we will need to put up a fix.

[puneetlath]

Ah whoops, understood.

@christianwen feel free to update your proposal accordingly if you're still interested!