Skip to content

Add performance monitoring configuration behind feature flags – Off by default #7831

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

Merged
merged 5 commits into from
Nov 1, 2022
+75 −4
Merged

Add performance monitoring configuration behind feature flags – Off by default #7831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
64ba63e
Initialize `WCCrashLoggingDataProvider` with a `FeatureFlagService`
mokagio Oct 11, 2022
e7727ff
Define `FeatureFlag` cases for performance monitoring
mokagio Oct 11, 2022
f37a36f
Add performance monitoring configuration via feature flags
mokagio Oct 11, 2022
461d7a4
Set performance monitoring to `false` in App Store builds
mokagio Oct 11, 2022
02d928c
Add `RELEASE-NOTES.txt` entry for performance monitoring flags
mokagio Oct 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Experiments/Experiments/DefaultFeatureFlagService.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,14 @@ public struct DefaultFeatureFlagService: FeatureFlagService {
return buildConfig == .localDeveloper || buildConfig == .alpha
case .productsOnboarding:
return buildConfig == .localDeveloper || buildConfig == .alpha
case .performanceMonitoring,
.performanceMonitoringCoreData,
.performanceMonitoringFileIO,
.performanceMonitoringNetworking,
.performanceMonitoringViewController,
.performanceMonitoringUserInteraction:
// Disabled by default to avoid costs spikes, unless in internal testing builds.
return buildConfig == .alpha
default:
return true
}
Expand Down
35 changes: 35 additions & 0 deletions Experiments/Experiments/FeatureFlag.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,39 @@ public enum FeatureFlag: Int {
/// Hides products onboarding development.
///
case productsOnboarding

// MARK: - Performance Monitoring
//
// These flags are not transient. That is, they are not here to help us rollout a feature,
// but to serve a safety switches to granularly turn off performance monitoring if it looks
// like we are consuming too many events.

/// Whether to enable performance monitoring.
///
case performanceMonitoring

/// Whether to enable performance monitoring for Core Data operations.
///
/// - Note: The app will ignore this if `performanceMonitoring` is `false`
case performanceMonitoringCoreData

/// Whether to enable performance monitoring for file IO operations.
///
/// - Note: The app will ignore this if `performanceMonitoring` is `false`
case performanceMonitoringFileIO

/// Whether to enable performance monitoring for networking operations.
///
/// - Note: The app will ignore this if `performanceMonitoring` is `false`
case performanceMonitoringNetworking

/// Whether to enable performance monitoring for user interaction events.
///
/// - Note: The app will ignore this if `performanceMonitoring` is `false`
case performanceMonitoringUserInteraction

/// Whether to enable performance monitoring for `UIViewController` life-cycle events.
///
/// - Note: The app will ignore this if `performanceMonitoring` is `false`.
case performanceMonitoringViewController
}
1 change: 1 addition & 0 deletions RELEASE-NOTES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

11.0
-----
- [internal] Add support for controlling performance monitoring via Sentry. **Off by default**. [https://github.com/woocommerce/woocommerce-ios/pull/7831]


10.9
Expand Down
4 changes: 3 additions & 1 deletion WooCommerce/Classes/ServiceLocator/ServiceLocator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ final class ServiceLocator {

/// Crash Logging Stack
///
private static var _crashLogging: CrashLoggingStack = WooCrashLoggingStack()
private static var _crashLogging: CrashLoggingStack = WooCrashLoggingStack(
featureFlagService: featureFlagService
)

/// Support for external Card Readers
///
Expand Down
31 changes: 28 additions & 3 deletions WooCommerce/Classes/Tools/Logging/WooCrashLoggingStack.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,15 @@ struct WooCrashLoggingStack: CrashLoggingStack {
let crashLogging: AutomatticTracks.CrashLogging
let eventLogging: EventLogging

private let crashLoggingDataProvider = WCCrashLoggingDataProvider()
private let crashLoggingDataProvider: WCCrashLoggingDataProvider
private let eventLoggingDataProvider = WCEventLoggingDataSource()
private let eventLoggingDelegate = WCEventLoggingDelegate()

init() {
init(featureFlagService: FeatureFlagService) {
let eventLogging = EventLogging(dataSource: eventLoggingDataProvider, delegate: eventLoggingDelegate)

self.eventLogging = eventLogging
self.crashLoggingDataProvider = WCCrashLoggingDataProvider(featureFlagService: featureFlagService)
self.crashLogging = AutomatticTracks.CrashLogging(dataProvider: crashLoggingDataProvider, eventLogging: eventLogging)

/// Upload any remaining files any time the app becomes active
Expand Down Expand Up @@ -87,7 +88,11 @@ class WCCrashLoggingDataProvider: CrashLoggingDataProvider {
/// Indicates that app is in an inconsistent state and we don't want to start asking it for metadata
fileprivate var appIsCrashing = false

init() {
let featureFlagService: FeatureFlagService

init(featureFlagService: FeatureFlagService) {
self.featureFlagService = featureFlagService

NotificationCenter.default.addObserver(self, selector: #selector(updateCrashLoggingSystem(_:)), name: .defaultAccountWasUpdated, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(updateCrashLoggingSystem(_:)), name: .logOutEventReceived, object: nil)
NotificationCenter.default.addObserver(self, selector: #selector(updateCrashLoggingSystem(_:)), name: .StoresManagerDidUpdateDefaultSite, object: nil)
Expand Down Expand Up @@ -129,6 +134,26 @@ class WCCrashLoggingDataProvider: CrashLoggingDataProvider {
ServiceLocator.crashLogging.setNeedsDataRefresh()
}
}

// MARK: – Performance Monitoring

var performanceTracking: PerformanceTracking {
guard featureFlagService.isFeatureFlagEnabled(.performanceMonitoring) else {
return .disabled
}

return .enabled(
.init(
// FIXME: Is there a way to control this via feature flags?
sampler: { 0.1 },
Comment on lines +147 to +148
Copy link
Contributor Author

@mokagio mokagio Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As long as our feature toggles are Bool only, the only way I can think to configure the sampler rate from the app is to have .performanceMonitoringSampleRateTenPercent, .performanceMonitoringSampleRateTwentyPercent, etc. and a method to compute the numeric value to use here based on a rule such as "use the value corresponding to the highest feature flag enabled".

Eg.

performanceMonitoringSampleRateTenPercent = true
performanceMonitoringSampleRateTwentyPercent = false
performanceMonitoringSampleRateThirtyPercent = true

// => sample rate = 0.3

This sounds like a lot of feature toggles and something it would be easy to get wrong because of how easy it is to forget about updating all of the toggles.

Copy link
Contributor

@crazytonyli crazytonyli Nov 1, 2022

Choose a reason for hiding this comment

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

I feel like it would be useful to have a remote config for this sample rate, so that we can adjust it in production environment if we find there are too many events sent to Sentry and we have to pay unnecessary cost for it. But as a local configuration though, I feel like it's almost not worth the effort 😿

Copy link
Contributor Author

@mokagio mokagio Nov 1, 2022

Choose a reason for hiding this comment

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

and something it would be easy to get wrong because of how easy it is to forget about updating all of the toggles.

My assessment on the risks of using a number of feature toggles to compute the sample rate might be greatly reduced once @hannahtinkler's feature toggles ship.

In particular, since the new feature toggles will be described in code, I can see a reverse of the computation logic described here in place where we give the backend a rate and it decides which toggles to serve as on and which off.

trackCoreData: featureFlagService.isFeatureFlagEnabled(.performanceMonitoringCoreData),
trackFileIO: featureFlagService.isFeatureFlagEnabled(.performanceMonitoringFileIO),
trackNetwork: featureFlagService.isFeatureFlagEnabled(.performanceMonitoringNetworking),
trackUserInteraction: featureFlagService.isFeatureFlagEnabled(.performanceMonitoringUserInteraction),
trackViewControllers: featureFlagService.isFeatureFlagEnabled(.performanceMonitoringViewController)
)
)
}
}

struct CrashLoggingSettings {
Expand Down