fix: 10622 android metrics crash

[zzz0108]

Details

This fixes the crash when we enable

ONYX_METRICS=true
CAPTURE_METRICS=true

Please refer to this accepted proposal for more details

Related Issues

Expensify/App#10622

Automated Tests

This is a bug fix, no new behavior is introduced so I didn't add tests.

Linked PRs

Will update when a new Onyx version with this change is released, I assume there'll be another PR to bump the Onyx version in https://github.com/Expensify/App

Screenshots

Screenshots showing no crash happens in any platform after the fix

Web

Mobile Web

Desktop

iOS

Android

[github-actions]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[zzz0108]

I have read the CLA Document and I hereby sign the CLA

[zzz0108]

recheck

[puneetlath]

@Santhosh-Sellavel and I should review this.

[Santhosh-Sellavel]

@christianwen

Seems this test is relevant to this issue, wondering should this test had to be failed earlier.

react-native-onyx/tests/unit/onyxMetricsDecorationTest.js

Lines 57 to 79 in 961320c

	it('Should decorate exposed methods', () => {
	// Given Onyx is initialized with `captureMetrics: true`
	Onyx.init({
	keys: ONYX_KEYS,
	registerStorageEventListener: jest.fn(),
	captureMetrics: true,
	});
	// When calling decorated methods through Onyx[methodName]
	const methods = ['set', 'multiSet', 'clear', 'merge', 'mergeCollection'];
	methods.forEach(name => Onyx[name]('mockKey', {mockKey: {mockValue: 'mockValue'}}));
	return waitForPromisesToResolve()
	.then(() => {
	// Then metrics should have captured data for each method
	const summaries = Onyx.getMetrics().summaries;
	methods.forEach((name) => {
	expect(summaries[`Onyx:${name}`].total).toBeGreaterThan(0);
	});
	});
	});
	});

Any thoughts?

cc: @puneetlath

[zzz0108]

@Santhosh-Sellavel that test doesn't fail because it's using the react-native implementation in lib/metrics/index.native.js, it's not using the noop in lib/metrics/index.web.js

[Santhosh-Sellavel]

So are we missing unit tests for the web?

[Santhosh-Sellavel]

@puneetlath Any thoughts on this #198 (comment)

[puneetlath]

Agreed - can we add a test for the web scenario?

[Santhosh-Sellavel]

@christianwen any update?

[zzz0108]

@Santhosh-Sellavel sorry I'll add the tests today, been a bit occupied this week

[zzz0108]

@Santhosh-Sellavel unit test added, thanks

[Santhosh-Sellavel]

Looks good! Will test on PR against E/App.
All you @puneetlath @aldo-expensify

[puneetlath]

@christianwen can you please merge main.

[zzz0108]

@puneetlath done, @Santhosh-Sellavel can give the review again. Please note there's the other merged PR #206 that has one of the change in this PR (the Onyx.update change), so I've removed that redundant change in this PR as part of the conflict resolution