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

RUMM-1086 LongTaskObserver implemented #567

Merged
merged 3 commits into from
Sep 21, 2021

Conversation

buranmert
Copy link
Contributor

@buranmert buranmert commented Aug 19, 2021

What and why?

Long Tasks are events that occur when the main thread is blocked for more than the given threshold.
This event can show our users whether their apps block the main thread longer than needed and if so, how and when.

How?

RunLoopObservers added to RunLoop.main: they observe the intervals between run loop activites and report long intervals.
These observers act similarly to view and user action tracking.

TODO

  • Add ObjC APIs

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@buranmert buranmert self-assigned this Aug 19, 2021
@buranmert buranmert force-pushed the buranmert/RUMM-1086-long-tasks branch from bf65589 to 767ca56 Compare August 19, 2021 13:27
@buranmert
Copy link
Contributor Author

Tested in Example and works fine, but RunLoops seem to work differently in unit tests and my case doesn't pass. I'll keep it as draft until I figure this out.

Copy link
Contributor Author

@buranmert buranmert left a comment

Choose a reason for hiding this comment

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

instead of fixing the problem in unit tests, we removed them in favor of integration tests ✅

Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

The actual code of observing long tasks (CFRunLoopObserver) looks fine 👍, but the way we collect LTs is missing few crucial elements which have no clear answers in proposed architecture:

These all will come with its own configuration, lifecycle, threading and architectural problems, all solved in our existing auto-instrumentation stacks for actions and resources:

As long tasks collection is technically no different than collection of actions and resources, IMO it is better to just re-use existing concepts and architecture for implementing LTs. Otherwise, we'll have to find new solutions to these known problems, through VitalReader.

@buranmert buranmert force-pushed the buranmert/RUMM-1086-long-tasks branch 2 times, most recently from e644f49 to 792a3a3 Compare September 16, 2021 09:53
@buranmert buranmert marked this pull request as ready for review September 16, 2021 10:01
@buranmert buranmert requested a review from a team as a code owner September 16, 2021 10:01
@buranmert buranmert force-pushed the buranmert/RUMM-1086-long-tasks branch 3 times, most recently from af2a194 to 85fa7aa Compare September 16, 2021 12:31
RunLoopObservers added to RunLoop.main
they observe the intervals between run loop activities
and report long intervals.
@buranmert buranmert force-pushed the buranmert/RUMM-1086-long-tasks branch from 85fa7aa to 602db06 Compare September 16, 2021 14:03
@ncreated ncreated self-requested a review September 16, 2021 15:53
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

🎯 💯 Looks great 👌. I left few small comments and one potential bug indication.

Even a simple button tap can take long (eg: 0.25s in my local)
Long task threshold is increased to 1 second in UI Tests
@buranmert buranmert changed the title RUMM-1086 VitalLongTaskReader implemented RUMM-1086 LongTaskObserver implemented Sep 20, 2021
@buranmert buranmert requested a review from ncreated September 20, 2021 15:21
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

🚀

@buranmert buranmert merged commit 6fc6d5d into master Sep 21, 2021
@buranmert buranmert deleted the buranmert/RUMM-1086-long-tasks branch September 21, 2021 13:48
@ncreated ncreated mentioned this pull request Sep 28, 2021
2 tasks
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.

2 participants