-
Notifications
You must be signed in to change notification settings - Fork 134
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
Conversation
bf65589
to
767ca56
Compare
Tested in |
There was a problem hiding this 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 ✅
Sources/Datadog/RUM/AutoInstrumentation/Actions/UIApplicationSwizzler.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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:
- LTs are part of optional auto-instrumentation, configured with a public API (
trackLongTasks()
in Android SDK); - LTs should not only be counted on
view.longTask.count
, but we should also send theLongTaskEvent
(see: Android SDK); - LTs should also be included in our data scrubbing API (through
RUMEventMapper
).
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:
RUMAutoInstrumentation
→RUMCommand
→RUMScope
.
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
.
e644f49
to
792a3a3
Compare
af2a194
to
85fa7aa
Compare
RunLoopObservers added to RunLoop.main they observe the intervals between run loop activities and report long intervals.
85fa7aa
to
602db06
Compare
There was a problem hiding this 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.
Sources/Datadog/RUM/AutoInstrumentation/LongTasks/LongTaskObserver.swift
Outdated
Show resolved
Hide resolved
Tests/DatadogIntegrationTests/Scenarios/RUM/RUMMobileVitalsScenarioTests.swift
Outdated
Show resolved
Hide resolved
Tests/DatadogIntegrationTests/Scenarios/RUM/RUMMobileVitalsScenarioTests.swift
Outdated
Show resolved
Hide resolved
Tests/DatadogIntegrationTests/Scenarios/RUM/RUMMobileVitalsScenarioTests.swift
Show resolved
Hide resolved
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
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?
RunLoopObserver
s added toRunLoop.main
: they observe the intervals between run loop activites and report long intervals.These observers act similarly to view and user action tracking.
TODO
Review checklist