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: UI event transactions for clicks #1784

Merged
merged 60 commits into from
May 23, 2022
Merged

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Apr 28, 2022

📜 Description

Creates transactions when the user clicks an UIButton, UISegmentedControl, UIBarButtonItem or UIPageControl. For more details, see getsentry/team-mobile#4.

💡 Motivation and Context

getsentry/team-mobile#4

💚 How did you test it?

Unit tests, simulator, and real devices.

📝 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

@github-actions
Copy link

github-actions bot commented Apr 28, 2022

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

Generated by 🚫 dangerJS against e56cdd9

@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2022

Codecov Report

Merging #1784 (e56cdd9) into master (1213bf0) will increase coverage by 0.56%.
The diff coverage is 99.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1784      +/-   ##
==========================================
+ Coverage   91.74%   92.30%   +0.56%     
==========================================
  Files         198      200       +2     
  Lines        9159     9388     +229     
==========================================
+ Hits         8403     8666     +263     
+ Misses        756      722      -34     
Impacted Files Coverage Δ
Sources/Sentry/include/SentryDependencyContainer.h 33.33% <0.00%> (-4.17%) ⬇️
Sources/Sentry/SentryCurrentDate.m 75.00% <75.00%> (ø)
Sources/Sentry/Public/SentryOptions.h 100.00% <100.00%> (ø)
Sources/Sentry/SentryBreadcrumbTracker.m 82.81% <100.00%> (ø)
Sources/Sentry/SentryDefaultCurrentDateProvider.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryDependencyContainer.m 91.54% <100.00%> (+0.64%) ⬆️
Sources/Sentry/SentryDispatchQueueWrapper.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryHub.m 98.65% <100.00%> (+0.06%) ⬆️
Sources/Sentry/SentryOptions.m 99.58% <100.00%> (+0.01%) ⬆️
Sources/Sentry/SentryPerformanceTracker.m 100.00% <100.00%> (ø)
... and 15 more

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 1213bf0...e56cdd9. Read the comment docs.

brustolin and others added 2 commits May 12, 2022 10:27
Creates transactions when a control sends an action to the application, like button clicks and switch toggles.

Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
@philipphofmann philipphofmann marked this pull request as ready for review May 18, 2022 08:31
@philipphofmann philipphofmann requested a review from brustolin May 19, 2022 07:33
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

Great work! A few more things we should talk about it.

Sources/Sentry/SentryTracer.m Show resolved Hide resolved
* convert the selector to a Swift appropriate format aligned with the Swift #selector syntax.
* method:first:second:third: gets converted to method(first:second:third:)
*/
- (NSString *)getTransactionName:(NSString *)action target:(id)target
Copy link
Contributor

@brustolin brustolin May 19, 2022

Choose a reason for hiding this comment

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

Here are some usage samples to simplify my explanation:

Swift Function Result How I think it should be to not cause confusion
func testClick() iOS_Swift.ViewController.testClick iOS_Swift.ViewController.testClick()
func testClick(_ sender : Any) iOS_Swift.ViewController.testClick iOS_Swift.ViewController.testClick(_:)
func testClick(sender : Any) iOS_Swift.ViewController.testClickWithSender iOS_Swift.ViewController.testClick(sender:)
func testClick(sender : Any, second : Any) iOS_Swift.ViewController.testClickWithSender(second:) iOS_Swift.ViewController.testClick(sender:second:)
func testClick(_ sender : Any, second : Any) iOS_Swift.ViewController.testClick(second:) iOS_Swift.ViewController.testClick(_:secondParam:)

As I said before, it is a little trickier to achieve perfect solution, Im kind fine with the result we have right now, because the first and second items are more common, but I do believe this will cause some confusion with some users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I get your point but aren't most of these events coming from the storyboard, and there we only have

  • func testClick()
  • func testClick(_ sender : Any)
  • func testClick(_ sender : Any, _ second : Any)

I agree that our solution is not optimal, but I think it's enough for you to figure out which code was executed. The problem is that only when looking at the action I don't know how to get the difference between func testClick(_ sender : Any) and func testClick() without using other objc_runtime functions. Do have any idea how we could solve this? Maybe we can also fix this before enabling enableUserInteractionTracing per default.

Copy link
Member Author

@philipphofmann philipphofmann May 20, 2022

Choose a reason for hiding this comment

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

Are you okay with creating an issue for this and fixing it later, @brustolin?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how to get the difference between func testClick(_ sender : Any) and func testClick()

if actions contain :, it has parameter, otherwise not.
But I thing we can release this way and wait for feedbacks

Sources/Sentry/SentryUIEventTracker.m Outdated Show resolved Hide resolved
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

I believe we already have a good feature. Let's merge it and gather feedbacks.

@philipphofmann philipphofmann merged commit 2d7cc7b into master May 23, 2022
@philipphofmann philipphofmann deleted the feat/ui-transactions branch May 23, 2022 14:49
armcknight pushed a commit that referenced this pull request Jul 1, 2022
Creates transactions when the user clicks an UIButton, UISegmentedControl, UIBarButtonItem, or UIPageControl.

Co-authored-by: Dhiogo Brustolin <dhiogorb@gmail.com>
armcknight pushed a commit that referenced this pull request Jul 1, 2022
Creates transactions when the user clicks an UIButton, UISegmentedControl, UIBarButtonItem, or UIPageControl.

Co-authored-by: Dhiogo Brustolin <dhiogorb@gmail.com>
@kevinrenskers kevinrenskers mentioned this pull request Sep 12, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants