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.

Sign up for GitHub

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

Jump to bottom

Feat: Slow/Frozen frames metrics #1609

Merged
merged 16 commits into from
Jul 28, 2021
Merged

Feat: Slow/Frozen frames metrics #1609

merged 16 commits into from
Jul 28, 2021

Conversation

marandaneto
Copy link
Contributor

@marandaneto marandaneto commented Jul 19, 2021 •
edited
Loading

📜 Description

Feat: Slow/Frozen frames metrics

💡 Motivation and Context

Closes #1466

💚 How did you test it?

📝 Checklist

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

🔮 Next steps

marandaneto
marandaneto commented Jul 20, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2021 •
edited
Loading

Codecov Report

Merging #1609 (5871e7c) into main (82dbee6) will decrease coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1609      +/-   ##
============================================
- Coverage     75.93%   75.93%   -0.01%     
- Complexity     2003     2004       +1     
============================================
  Files           202      202              
  Lines          6939     6943       +4     
  Branches        690      691       +1     
============================================
+ Hits           5269     5272       +3     
- Misses         1334     1335       +1     
  Partials        336      336
Impacted Files Coverage Δ
sentry/src/main/java/io/sentry/HubAdapter.java 5.08% <ø> (ø)
sentry/src/main/java/io/sentry/IHub.java 90.47% <ø> (ø)
sentry/src/main/java/io/sentry/NoOpHub.java 47.50% <ø> (ø)
sentry/src/main/java/io/sentry/Sentry.java 41.49% <ø> (ø)
...main/java/io/sentry/protocol/MeasurementValue.java 0.00% <0.00%> (ø)
sentry/src/main/java/io/sentry/SentryTracer.java 92.85% <83.33%> (+0.19%) ⬆️
sentry/src/main/java/io/sentry/Hub.java 73.00% <100.00%> (ø)
sentry/src/main/java/io/sentry/Span.java 91.22% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82dbee6...5871e7c. Read the comment docs.

philipphofmann
philipphofmann reviewed Jul 20, 2021
View reviewed changes
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.

Looks already good. I added a few comments.

sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesState.java Outdated Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesState.java Outdated Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesState.java Outdated Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java Outdated Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/AppStartState.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/TransactionListener.java Outdated Show resolved Hide resolved
philipphofmann
philipphofmann reviewed Jul 20, 2021
View reviewed changes
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.

Just one open comment about the naming. Is it hard to add tests for ActivityFramesTracker or how is it tested?

sentry/src/main/java/io/sentry/TransactionListener.java Outdated Show resolved Hide resolved
bruno-garcia
bruno-garcia reviewed Jul 20, 2021
View reviewed changes
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Just a couple notes (given it's a draft)

sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java Outdated Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/TransactionListener.java Outdated Show resolved Hide resolved
philipphofmann
philipphofmann reviewed Jul 20, 2021
View reviewed changes
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.

One tiny comment, and is it possible to add tests for ActivityFramesTracker?

sentry/src/main/java/io/sentry/TransactionFinishedCallback.java Outdated Show resolved Hide resolved
@marandaneto marandaneto mentioned this pull request Jul 27, 2021
Closed
4 tasks
@marandaneto marandaneto marked this pull request as ready for review July 27, 2021 14:52
bruno-garcia
bruno-garcia approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Other than some small suggestions LGTM! 🚀

CHANGELOG.md Outdated Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ActivityFramesTracker.java Outdated Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java Show resolved Hide resolved
sentry-android-core/src/main/java/io/sentry/android/core/PerformanceAndroidEventProcessor.java Show resolved Hide resolved
...ry-android-core/src/test/java/io/sentry/android/core/PerformanceAndroidEventProcessorTest.kt Show resolved Hide resolved
@marandaneto
Copy link
Contributor Author

Other than some small suggestions LGTM! 🚀

applied them and added one more test too, thanks

@marandaneto marandaneto merged commit b364644 into main Jul 28, 2021
@marandaneto marandaneto deleted the feat/slow-frozen-frames branch July 28, 2021 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bruno-garcia bruno-garcia bruno-garcia approved these changes

@maciejwalkowiak maciejwalkowiak Awaiting requested review from maciejwalkowiak

@jennmueng jennmueng Awaiting requested review from jennmueng

@philipphofmann philipphofmann Awaiting requested review from philipphofmann

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marandaneto @codecov-commenter @bruno-garcia @philipphofmann @jennmueng