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: add Sentry screenName tracking #4646

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

m1entus
Copy link

@m1entus m1entus commented Dec 18, 2024

📜 Description

Added UIViewController custom screenName tracking ref: #4642

💡 Motivation and Context

I used

protocol SentryViewControllerBreadcrumbTracking {
    var screenName: String { get }
}

If viewController responds to SentryViewControllerBreadcrumbTracking then use screenName instead of [SwiftDescriptor getObjectClassName:controller]

💚 How did you test it?

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.590%. Comparing base (b7c9edb) to head (efd328d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...ons/Breadcrumbs/SentryBreadcrumbTrackerTests.swift 88.636% 5 Missing ⚠️
Sources/Sentry/SentryBreadcrumbTracker.m 90.000% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##              main     #4646   +/-   ##
=========================================
  Coverage   90.590%   90.590%           
=========================================
  Files          620       620           
  Lines        71075     71118   +43     
  Branches     25311     25299   -12     
=========================================
+ Hits         64387     64426   +39     
- Misses        6596      6598    +2     
- Partials        92        94    +2     
Files with missing lines Coverage Δ
Sources/Sentry/SentryBreadcrumbTracker.m 88.405% <90.000%> (+0.782%) ⬆️
...ons/Breadcrumbs/SentryBreadcrumbTrackerTests.swift 94.687% <88.636%> (-0.965%) ⬇️

... and 9 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

@m1entus m1entus requested a review from brustolin December 18, 2024 12:56
@kahest kahest linked an issue Dec 18, 2024 that may be closed by this pull request
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.

Thanks for the contribution @m1entus.

LGTM

@@ -9,6 +9,7 @@
### Features

- Show session replay options as replay tags (#4639)
- Add UIViewController custom screenName for breadcrumb tracking (#4642)
Copy link
Member

Choose a reason for hiding this comment

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

So it matches the PR number.

Suggested change
- Add UIViewController custom screenName for breadcrumb tracking (#4642)
- Add UIViewController custom screenName for breadcrumb tracking (#4646)

/// UIViewController performance tracker.
///
@objc
public protocol SentryViewControllerBreadcrumbTracking: NSObjectProtocol {
Copy link
Member

Choose a reason for hiding this comment

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

h: We use SwiftDescriptor getObjectClassName in multiple locations in our code base for getting the UIViewControllerName; see https://github.com/search?q=repo%3Agetsentry%2Fsentry-cocoa%20SwiftDescriptor%20getObjectClassName&type=code

I think we should use SentryViewControllerBreadcrumbTracking in all of these scenarios for consistency. Therefore, we should rename this to SentryUIViewControllerName and update the code comment above accordingly.

Copy link
Author

@m1entus m1entus Dec 19, 2024

Choose a reason for hiding this comment

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

Ok i will change, i was thinking about some generic protocol in case in future you would like to add some additional parameters which might be send with breadcrumb. If we name it UIViewControllerName and in future we would like to add additional parameters which might, propably we would have to rename it? What about SentryUIViewControllerDescriptionProviding or SentryUIViewControllerDescriptor ?

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.

Allow to use custom UIViewController name when tracking breadcumbs
3 participants