-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: implement comprehensive Sentry metrics strategy #83
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
Conversation
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.
Pull request overview
This PR implements a comprehensive migration from Sentry analytics events to the Sentry Metrics API, introducing a centralized SentryMetricsHelper utility for type-safe metric tracking across the application. The change improves analytics aggregation capabilities while maintaining the project's privacy-first approach of tracking only entity UUIDs and behavior patterns without PII.
Changes:
- Introduced
SentryMetricsHelperwith type-safe wrapper functions for all common metric patterns (creation, sharing, customization, performance, NFC, search) - Replaced
SentrySDK.capture(event:)calls withSentrySDK.metrics.count()for user action tracking across all UI components - Added QR code performance metrics tracking cache hit/miss/eviction and generation duration using
CFAbsoluteTimeGetCurrent()for precise timing - Implemented search analytics, share extension state tracking, and link/list management metrics
- Enabled Sentry metrics in both main app and share extension configurations
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Targets/App/Sources/Utils/SentryMetricsHelper.swift | New centralized helper with type-safe wrappers for all metric patterns |
| Targets/ShareExtension/Sources/ShareViewController.swift | Added share extension metrics tracking with state management for user interactions |
| Targets/App/Sources/UI/LinkLists/LinkListsContainerView.swift | Added search and list deletion metrics |
| Targets/App/Sources/UI/LinkListDetail/LinkListDetailContainerView.swift | Added search and link deletion metrics |
| Targets/App/Sources/UI/LinkListInfo/LinkListInfoContainerView.swift | Migrated customization tracking to metrics |
| Targets/App/Sources/UI/LinkInfo/LinkInfoContainerView.swift | Migrated customization tracking to metrics |
| Targets/App/Sources/UI/LinkDetailNFCSharing/LinkDetailNFCSharingViewModel.swift | Added NFC availability and success/failure metrics |
| Targets/App/Sources/UI/LinkDetail/LinkDetailRenderView.swift | Migrated sharing metrics |
| Targets/App/Sources/UI/LinkDetail/LinkDetailContainerView.swift | Added QR code performance metrics and migrated sharing/opening metrics |
| Targets/App/Sources/UI/CreateListEditor/CreateLinkListEditorContainerView.swift | Migrated creation metrics |
| Targets/App/Sources/UI/CreateLinkWithoutListEditor/CreateLinkWithListPickerEditorContainerView.swift | Migrated creation metrics |
| Targets/App/Sources/UI/CreateLinkEditor/CreateLinkEditorContainerView.swift | Migrated creation metrics |
| Targets/App/Sources/Services/QRCodeCache.swift | Added cache hit/miss/eviction and memory warning metrics |
| Targets/App/Sources/Services/DataSeedingService.swift | Migrated database seeding metrics |
| Targets/App/Sources/Main/FlinkyApp.swift | Enabled metrics and migrated feedback form metrics |
| Documentation/Analytics.md | Comprehensive documentation update with metrics strategy, taxonomy, and examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Targets/App/Sources/UI/LinkListDetail/LinkListDetailContainerView.swift
Outdated
Show resolved
Hide resolved
- Add SentryMetricsHelper with type-safe metric tracking functions - Implement QR code cache hit/miss/eviction metrics - Track QR code generation duration with accurate timing (CFAbsoluteTimeGetCurrent) - Add search tracking for lists and links views - Implement share extension completion and cancellation tracking - Add list pinning/unpinning metrics - Track link/list deletion (single and bulk) with context - Implement NFC availability and sharing metrics - Add link opened tracking with link ID attribute - Track memory warnings with cache size context - Remove redundant app lifecycle metrics (handled by Sentry Sessions) - Update all analytics calls to use metrics instead of events This implements the metrics strategy outlined in Documentation/Analytics.md, providing comprehensive insights while maintaining privacy compliance.
b036b1b to
6b79f49
Compare
…ntainerView and LinkListsContainerView
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.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let cacheSize = storage.countLimit | ||
| let appState = UIApplication.shared.applicationState == .active ? "foreground" : "background" | ||
| SentryMetricsHelper.trackMemoryWarningReceived(cacheSizeAtWarning: cacheSize, appState: appState) |
Copilot
AI
Jan 21, 2026
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 cacheSizeAtWarning parameter receives storage.countLimit (the cache limit, which is 100), not the actual current cache size. NSCache doesn't expose its current count, so this metric will always report the maximum limit (100), not the actual number of cached items at the time of the warning. This makes the metric less useful for understanding cache pressure.
Consider either removing this parameter or documenting that it represents the cache limit rather than the actual size.
| let cacheSize = storage.countLimit | |
| let appState = UIApplication.shared.applicationState == .active ? "foreground" : "background" | |
| SentryMetricsHelper.trackMemoryWarningReceived(cacheSizeAtWarning: cacheSize, appState: appState) | |
| // NSCache does not expose the current item count, so we record the configured | |
| // cache limit at the time of the warning instead of the actual cache size. | |
| let cacheLimitAtWarning = storage.countLimit | |
| let appState = UIApplication.shared.applicationState == .active ? "foreground" : "background" | |
| SentryMetricsHelper.trackMemoryWarningReceived(cacheSizeAtWarning: cacheLimitAtWarning, appState: appState) |
| SentryMetricsHelper.trackSearchQueryLength(length: newValue.count, searchContext: "links") | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 21, 2026
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 assignment previousSearchText = oldValue on line 179 serves no purpose since previousSearchText is never read. This line can be removed.
| } |
| SentryMetricsHelper.trackLinkDeleted(listLinkCount: remainingLinkCount) | ||
| } catch { |
Copilot
AI
Jan 21, 2026
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.
When tracking list deletion, the linkCount is captured before the list is deleted. However, when tracking link deletion on lines 40-42, remainingLinkCount is calculated as list.links.count - 1, which assumes the deletion hasn't happened yet but the count should reflect the state after deletion. This is inconsistent with the list deletion tracking pattern. Consider using list.links.count instead if you want to track the count before deletion, or ensure the metric name accurately reflects what's being tracked (e.g., "links_remaining_after_deletion").
| SentryMetricsHelper.trackLinkDeleted(listLinkCount: remainingLinkCount) | |
| } catch { | |
| let listLinkCount = list.links.count | |
| SentryMetricsHelper.trackLinkDeleted(listLinkCount: listLinkCount) |
| Self.logger.debug("QR code cache evicting object due to memory pressure") | ||
| SentryMetricsHelper.trackQRCodeCacheEviction(reason: "size_limit") |
Copilot
AI
Jan 21, 2026
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 eviction tracking always uses reason "size_limit", but this NSCache delegate method is called for any eviction, including those due to memory pressure. The actual reason could be either memory pressure or cache size limit. Consider either removing the reason attribute since it can't be accurately determined, or tracking evictions separately from memory warnings (which are already tracked via handleMemoryWarning).
| Self.logger.debug("QR code cache evicting object due to memory pressure") | |
| SentryMetricsHelper.trackQRCodeCacheEviction(reason: "size_limit") | |
| Self.logger.debug("QR code cache evicting object from cache") | |
| SentryMetricsHelper.trackQRCodeCacheEviction(reason: "unknown") |
| "list_selected": wasListSelected, | ||
| "name_edited": wasNameEdited |
Copilot
AI
Jan 21, 2026
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 attribute values for list_selected and name_edited should be strings ("true" or "false"), but they are being passed as Bool values directly. Sentry metrics require all attribute values to be strings. This should either use String(wasListSelected) and String(wasNameEdited) like in the helper function, or call the helper function SentryMetricsHelper.trackShareExtensionCompleted(listSelected: wasListSelected, nameEdited: wasNameEdited) instead of directly using the Sentry API.
| "list_selected": wasListSelected, | |
| "name_edited": wasNameEdited | |
| "list_selected": String(wasListSelected), | |
| "name_edited": String(wasNameEdited) |
| import Foundation | ||
| import Network | ||
| import os.log | ||
| import Sentry | ||
| import UIKit | ||
|
|
||
| /// Observes app health signals and reports them as Sentry metrics. | ||
| /// | ||
| /// This class monitors: | ||
| /// - **Thermal state changes**: Track when the device heats up or cools down | ||
| /// - **Network reachability changes**: Track connectivity state transitions | ||
| /// - **App state transitions**: Track foreground/background transitions | ||
| /// | ||
| /// These metrics are recommended by the Sentry SDK team for app health monitoring | ||
| /// that doesn't overlap with automatic tracing features. | ||
| /// | ||
| /// Reference: https://github.com/getsentry/sentry-cocoa/issues/7000 | ||
| final class AppHealthObserver { | ||
|
|
||
| // MARK: - Singleton | ||
|
|
||
| /// Shared instance of the app health observer. | ||
| static let shared = AppHealthObserver() | ||
|
|
||
| // MARK: - Properties | ||
|
|
||
| private static let logger = Logger(subsystem: Bundle.main.bundleIdentifier ?? "Flinky", category: "AppHealthObserver") | ||
|
|
||
| private var previousThermalState: ProcessInfo.ThermalState = ProcessInfo.processInfo.thermalState | ||
| private var previousAppState: UIApplication.State = .inactive | ||
| private var previousNetworkStatus: NWPath.Status? | ||
| private var previousNetworkInterface: String? | ||
|
|
||
| private let networkMonitor = NWPathMonitor() | ||
| private let networkQueue = DispatchQueue(label: "com.flinky.network-monitor") | ||
|
|
||
| private var isObserving = false | ||
|
|
||
| // MARK: - Initialization | ||
|
|
||
| private init() {} | ||
|
|
||
| // MARK: - Public Methods | ||
|
|
||
| /// Starts observing app health signals. | ||
| /// | ||
| /// Call this method once during app initialization, typically in `FlinkyApp.init()`. | ||
| func startObserving() { | ||
| guard !isObserving else { | ||
| Self.logger.warning("AppHealthObserver is already observing") | ||
| return | ||
| } | ||
|
|
||
| isObserving = true | ||
| Self.logger.info("Starting app health observation") | ||
|
|
||
| setupThermalStateObserver() | ||
| setupNetworkReachabilityObserver() | ||
| setupAppStateObserver() | ||
| } | ||
|
|
||
| /// Stops observing app health signals. | ||
| /// | ||
| /// Call this method during app cleanup if needed. | ||
| func stopObserving() { | ||
| guard isObserving else { return } | ||
|
|
||
| isObserving = false | ||
| Self.logger.info("Stopping app health observation") | ||
|
|
||
| // swiftlint:disable:next notification_center_detachment | ||
| NotificationCenter.default.removeObserver(self) | ||
| networkMonitor.cancel() | ||
| } | ||
|
|
||
| // MARK: - Thermal State Observation | ||
|
|
||
| private func setupThermalStateObserver() { | ||
| previousThermalState = ProcessInfo.processInfo.thermalState | ||
|
|
||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(handleThermalStateChange), | ||
| name: ProcessInfo.thermalStateDidChangeNotification, | ||
| object: nil | ||
| ) | ||
|
|
||
| Self.logger.debug("Thermal state observer setup complete. Initial state: \(self.thermalStateString(self.previousThermalState))") | ||
| } | ||
|
|
||
| @objc private func handleThermalStateChange() { | ||
| let newState = ProcessInfo.processInfo.thermalState | ||
| let oldState = previousThermalState | ||
|
|
||
| guard newState != oldState else { return } | ||
|
|
||
| let fromString = thermalStateString(oldState) | ||
| let toString = thermalStateString(newState) | ||
| let isEscalation = newState.rawValue > oldState.rawValue | ||
|
|
||
| Self.logger.info("Thermal state changed: \(fromString) → \(toString) (escalation: \(isEscalation))") | ||
|
|
||
| // Track thermal state transition | ||
| SentryMetricsHelper.trackThermalStateTransition( | ||
| fromState: fromString, | ||
| toState: toString, | ||
| isEscalation: isEscalation | ||
| ) | ||
|
|
||
| // Add breadcrumb for debugging context | ||
| let breadcrumb = Breadcrumb(level: isEscalation ? .warning : .info, category: "device_health") | ||
| breadcrumb.message = "Thermal state changed: \(fromString) → \(toString)" | ||
| breadcrumb.data = [ | ||
| "from_state": fromString, | ||
| "to_state": toString, | ||
| "is_escalation": isEscalation | ||
| ] | ||
| SentrySDK.addBreadcrumb(breadcrumb) | ||
|
|
||
| previousThermalState = newState | ||
| } | ||
|
|
||
| private func thermalStateString(_ state: ProcessInfo.ThermalState) -> String { | ||
| SentryMetricsHelper.thermalStateString(state) | ||
| } | ||
|
|
||
| // MARK: - Network Reachability Observation | ||
|
|
||
| private func setupNetworkReachabilityObserver() { | ||
| networkMonitor.pathUpdateHandler = { [weak self] path in | ||
| self?.handleNetworkPathUpdate(path) | ||
| } | ||
|
|
||
| networkMonitor.start(queue: networkQueue) | ||
|
|
||
| Self.logger.debug("Network reachability observer setup complete") | ||
| } | ||
|
|
||
| private func handleNetworkPathUpdate(_ path: NWPath) { | ||
| let status = path.status | ||
| let interfaceType = getInterfaceType(path) | ||
| let statusString = status == .satisfied ? "connected" : "disconnected" | ||
|
|
||
| // Only track changes, not initial state | ||
| guard previousNetworkStatus != nil else { | ||
| previousNetworkStatus = status | ||
| previousNetworkInterface = interfaceType | ||
| Self.logger.debug("Initial network state: \(statusString) via \(interfaceType)") | ||
| return | ||
| } | ||
|
|
||
| // Check if there's an actual change | ||
| let statusChanged = previousNetworkStatus != status | ||
| let interfaceChanged = previousNetworkInterface != interfaceType | ||
|
|
||
| guard statusChanged || interfaceChanged else { return } | ||
|
|
||
| Self.logger.info("Network changed: \(statusString) via \(interfaceType) (expensive: \(path.isExpensive), constrained: \(path.isConstrained))") | ||
|
|
||
| // Track network reachability change | ||
| SentryMetricsHelper.trackNetworkReachabilityChanged( | ||
| status: statusString, | ||
| interfaceType: interfaceType, | ||
| isExpensive: path.isExpensive, | ||
| isConstrained: path.isConstrained | ||
| ) | ||
|
|
||
| // Add breadcrumb for debugging context | ||
| let breadcrumb = Breadcrumb(level: .info, category: "network") | ||
| breadcrumb.message = "Network changed: \(statusString) via \(interfaceType)" | ||
| breadcrumb.data = [ | ||
| "status": statusString, | ||
| "interface": interfaceType, | ||
| "is_expensive": path.isExpensive, | ||
| "is_constrained": path.isConstrained | ||
| ] | ||
| SentrySDK.addBreadcrumb(breadcrumb) | ||
|
|
||
| previousNetworkStatus = status | ||
| previousNetworkInterface = interfaceType | ||
| } | ||
|
|
||
| private func getInterfaceType(_ path: NWPath) -> String { | ||
| if path.usesInterfaceType(.wifi) { | ||
| return "wifi" | ||
| } else if path.usesInterfaceType(.cellular) { | ||
| return "cellular" | ||
| } else if path.usesInterfaceType(.wiredEthernet) { | ||
| return "wired" | ||
| } else if path.usesInterfaceType(.loopback) { | ||
| return "loopback" | ||
| } else { | ||
| return "other" | ||
| } | ||
| } | ||
|
|
||
| // MARK: - App State Observation | ||
|
|
||
| private func setupAppStateObserver() { | ||
| previousAppState = UIApplication.shared.applicationState | ||
|
|
||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(handleAppDidBecomeActive), | ||
| name: UIApplication.didBecomeActiveNotification, | ||
| object: nil | ||
| ) | ||
|
|
||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(handleAppWillResignActive), | ||
| name: UIApplication.willResignActiveNotification, | ||
| object: nil | ||
| ) | ||
|
|
||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(handleAppDidEnterBackground), | ||
| name: UIApplication.didEnterBackgroundNotification, | ||
| object: nil | ||
| ) | ||
|
|
||
| NotificationCenter.default.addObserver( | ||
| self, | ||
| selector: #selector(handleAppWillEnterForeground), | ||
| name: UIApplication.willEnterForegroundNotification, | ||
| object: nil | ||
| ) | ||
|
|
||
| Self.logger.debug("App state observer setup complete. Initial state: \(self.appStateString(self.previousAppState))") | ||
| } | ||
|
|
||
| @objc private func handleAppDidBecomeActive() { | ||
| trackAppStateTransition(to: .active) | ||
| } | ||
|
|
||
| @objc private func handleAppWillResignActive() { | ||
| trackAppStateTransition(to: .inactive) | ||
| } | ||
|
|
||
| @objc private func handleAppDidEnterBackground() { | ||
| trackAppStateTransition(to: .background) | ||
| } | ||
|
|
||
| @objc private func handleAppWillEnterForeground() { | ||
| // This is called before didBecomeActive, we'll track the full transition there | ||
| Self.logger.debug("App will enter foreground") | ||
| } | ||
|
|
||
| private func trackAppStateTransition(to newState: UIApplication.State) { | ||
| let oldState = previousAppState | ||
|
|
||
| guard newState != oldState else { return } | ||
|
|
||
| let fromString = appStateString(oldState) | ||
| let toString = appStateString(newState) | ||
|
|
||
| Self.logger.info("App state changed: \(fromString) → \(toString)") | ||
|
|
||
| // Track app state transition | ||
| SentryMetricsHelper.trackAppStateTransition(toState: toString, fromState: fromString) | ||
|
|
||
| previousAppState = newState | ||
| } | ||
|
|
||
| private func appStateString(_ state: UIApplication.State) -> String { | ||
| SentryMetricsHelper.appStateString(state) | ||
| } | ||
|
|
||
| // MARK: - Cleanup | ||
|
|
||
| deinit { | ||
| stopObserving() | ||
| } | ||
| } |
Copilot
AI
Jan 21, 2026
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 new AppHealthObserver service lacks test coverage. Since the repository has comprehensive testing for other services (e.g., QRCodeCacheTests, DataSeedingServiceTests), consider adding tests to verify:
- Singleton initialization
- Start/stop observing lifecycle
- Proper tracking of state transitions (avoiding duplicate metrics)
- Network path interface type detection
- Memory warning notification handling
This is particularly important for a service that automatically observes system-level notifications and could potentially send duplicate metrics.
| The following metrics are now implemented via `AppHealthObserver`: | ||
|
|
||
| - ✅ `memory.warning.received` (Counter) - Track memory warnings | ||
| - Attributes: `cache_size_at_warning` (number), `app_state` (string) |
Copilot
AI
Jan 21, 2026
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 documentation states that cache_size_at_warning is of type "number", but Sentry metrics require all attributes to be strings. The implementation correctly converts it to a string using String(cacheSizeAtWarning), so the documentation should reflect this as "string" not "number".
| - Attributes: `cache_size_at_warning` (number), `app_state` (string) | |
| - Attributes: `cache_size_at_warning` (string), `app_state` (string) |
| // Track NFC availability (not available) | ||
| SentrySDK.metrics.gauge( | ||
| key: "nfc.available", | ||
| value: 0.0, | ||
| unit: .generic("capability") | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| // Track NFC availability (available) | ||
| SentrySDK.metrics.gauge( | ||
| key: "nfc.available", | ||
| value: 1.0, | ||
| unit: .generic("capability") | ||
| ) | ||
|
|
Copilot
AI
Jan 21, 2026
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 gauge metric nfc.available is set every time the user initiates NFC sharing, which will repeatedly record the same device capability. Gauge metrics are typically used for values that change over time. Consider either:
- Recording this once at app launch or first NFC attempt
- Using a counter with attributes to track NFC initiation attempts on capable vs. non-capable devices
- Removing this metric entirely since device capability doesn't change
The current implementation will generate redundant gauge updates every time a user tries to share via NFC.
| // Track NFC availability (not available) | |
| SentrySDK.metrics.gauge( | |
| key: "nfc.available", | |
| value: 0.0, | |
| unit: .generic("capability") | |
| ) | |
| return | |
| } | |
| // Track NFC availability (available) | |
| SentrySDK.metrics.gauge( | |
| key: "nfc.available", | |
| value: 1.0, | |
| unit: .generic("capability") | |
| ) | |
| return | |
| } |
| SentryMetricsHelper.trackSearchQueryLength(length: newValue.count, searchContext: "lists") | ||
| } | ||
| } | ||
| } |
Copilot
AI
Jan 21, 2026
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 assignment previousSearchText = oldValue on line 211 serves no purpose since previousSearchText is never read. This line can be removed.
| } |
| /// Returns current cache size metrics for tracking | ||
| var currentCacheSize: Int { | ||
| // NSCache doesn't expose current count, so we return the limit | ||
| // Actual tracking happens via hit/miss/eviction metrics | ||
| return storage.countLimit | ||
| } | ||
|
|
Copilot
AI
Jan 21, 2026
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 currentCacheSize property was added but is never used in the codebase. Since NSCache doesn't expose the current count anyway (as noted in the comment), and the actual tracking happens via hit/miss/eviction metrics, this property should be removed to avoid confusion.
| /// Returns current cache size metrics for tracking | |
| var currentCacheSize: Int { | |
| // NSCache doesn't expose current count, so we return the limit | |
| // Actual tracking happens via hit/miss/eviction metrics | |
| return storage.countLimit | |
| } |
Summary
This PR implements the comprehensive Sentry metrics strategy outlined in
Documentation/Analytics.md, replacing analytics events with Sentry Metrics API for better aggregation and reduced noise.Changes
New Features
Improvements
Date()withCFAbsoluteTimeGetCurrent()for precise performance measurementsSentryMetricsHelperfor consistencyRemoved
Testing
Documentation
Updated
Documentation/Analytics.mdwith implementation details and examples.