Skip to content
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

Capture metrics using react-native-performance #101

Merged
merged 7 commits into from
Aug 23, 2021

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented Aug 18, 2021

Details

We're already using react-native-performance in Expensify/App
Using it here allows us to visualize Onyx measures in Flipper

image

Related Issues

Expensify/App#4656 (comment)

Automated Tests

Covered by existing tests

Linked PRs

@kidroca kidroca requested a review from a team as a code owner August 18, 2021 22:24
@MelvinBot MelvinBot requested review from timszot and removed request for a team August 18, 2021 22:24
@timszot
Copy link
Contributor

timszot commented Aug 19, 2021

This looks good to me, but I'd like @marcaaron to take a look here since I know he's more involved in the performance metrics.

Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but I spotted a few places where I think it couldn't hurt to add some additional context or clarifying code comments to make it easier to work on for the next person.

lib/metrics.js Show resolved Hide resolved
lib/metrics.js Outdated Show resolved Hide resolved
lib/metrics.js Show resolved Hide resolved
Since performance entries are created with `react-native-performance`
they no longer contain `endTime`.
End time is derived using `call.startTime` + `call.duration`
@kidroca kidroca requested a review from marcaaron August 23, 2021 15:15
@kidroca
Copy link
Contributor Author

kidroca commented Aug 23, 2021

Addressed requested changes. Ready for review

@marcaaron marcaaron merged commit 6eadd7f into Expensify:master Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants