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

feat: upload list of slow/frozen rendered frame timestamps during a profile #1910

Merged
merged 43 commits into from Jul 13, 2022

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Jun 21, 2022

📜 Description

Upload the start and end timestamp for each frame that is rendered while a transaction is being traced with profiling enabled.

💡 Motivation and Context

In order to help diagnose why an app may be experiencing janky frames, it would be helpful to be able to drill down into sampled backtraces in the web dashboard profiling area that corresponds to frames that take too long to render.

💚 How did you test it?

So far, just confirming that the data is uploaded by inspecting the app state in the debugging after manually working in the iOS-Swift sample app.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

  • Add scenario to iOS-Swift that forces slow/frozen frames

@github-actions
Copy link

github-actions bot commented Jun 21, 2022

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 25b8904

@armcknight armcknight requested a review from a team June 21, 2022 20:55
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Generally speaking, this PR goes in the right direction. Maybe it only makes sense to send the frames' timestamps not rendered in time. The server then could calculate the timestamps if we also send the start time of the first frame. This would reduce the payload significantly.

If we use a ring buffer for keeping track of the frame timestamps, we could already set the array to a fixed size, and we will avoid allocations for every frame.

Sources/Sentry/Public/SentryScreenFrames.h Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentryScreenFrames.h Outdated Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

First pass. Still need to review a few files.

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Show resolved Hide resolved
@armcknight armcknight changed the title feat: upload list of all rendered frame timestamps during a profile feat: upload list of slow/frozen rendered frame timestamps during a profile Jun 27, 2022
Sources/Sentry/SentryNSArrayRingBuffer.h Outdated Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryNSArrayRingBuffer.m Outdated Show resolved Hide resolved
Sources/Sentry/include/SentryFramesTracker.h Show resolved Hide resolved
@armcknight armcknight force-pushed the armcknight/gpu-frame-metrics branch 3 times, most recently from 6cd891d to 77cb781 Compare July 1, 2022 00:02
@armcknight armcknight marked this pull request as ready for review July 1, 2022 03:26
@armcknight armcknight force-pushed the armcknight/gpu-frame-metrics branch from 841ba7b to 91b136e Compare July 1, 2022 03:29
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

With #1912 we plan on adding spans for slow and frozen frames. After finishing this PR, this is going to be easy 👌.

This PR misses some tests.

Sources/Sentry/SentryFramesTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Outdated Show resolved Hide resolved
Sources/Sentry/include/SentryFramesTracker.h Show resolved Hide resolved
@philipphofmann
Copy link
Member

Sorry, @armcknight didn't get to this today. Will do it tomorrow.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Can we please add some tests @armcknight?

Sources/Sentry/Public/SentryScreenFrames.h Show resolved Hide resolved
Sources/Sentry/SentryFramesTracker.m Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Outdated Show resolved Hide resolved
@armcknight armcknight force-pushed the armcknight/gpu-frame-metrics branch from 61ddb40 to d2dfd83 Compare July 12, 2022 01:00
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/Sentry/Public/SentryScreenFrames.h Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Show resolved Hide resolved
@armcknight armcknight merged commit c0397aa into master Jul 13, 2022
@armcknight armcknight deleted the armcknight/gpu-frame-metrics branch July 13, 2022 19:34
kevinrenskers added a commit that referenced this pull request Jul 14, 2022
* master:
  ci: Don't run benchmarks on release (#1971)
  Don't track OOMs for simulators (#1970)
  feat: Automatic nest new spans with the ui life cycle function (#1959)
  docs: update some docs/comments to read a little better (#1966)
  ci: benchmarking updates (#1926)
  feat: upload list of slow/frozen rendered frame timestamps during a profile (#1910)
  feat: Enhance the UIViewController breadcrumbs with more data (#1945)

# Conflicts:
#	Sources/SentryCrash/Recording/Monitors/SentryCrashMonitor_System.h
kevinrenskers added a commit that referenced this pull request Jul 18, 2022
* master:
  ref: Fix linter error (#1981)
  fix: read free_memory when the event is captured, not only at SDK startup (#1962)
  fix: Remove Sentry keys from cached HTTP request headers (#1975)
  release: 7.21.0
  ci: Don't run benchmarks on release (#1971)
  Don't track OOMs for simulators (#1970)
  feat: Automatic nest new spans with the ui life cycle function (#1959)
  docs: update some docs/comments to read a little better (#1966)
  ci: benchmarking updates (#1926)
  feat: upload list of slow/frozen rendered frame timestamps during a profile (#1910)
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.

None yet

3 participants