Add performance monitoring configuration behind feature flags – Off by default #7831
Conversation
mokagio
added
category: tooling
| case .performanceMonitoring, | ||
| .performanceMonitoringCoreData, | ||
| .performanceMonitoringFileIO, | ||
| .performanceMonitoringNetworking, | ||
| .performanceMonitoringViewController, | ||
| .performanceMonitoringUserInteraction: | ||
| return buildConfig != .appStore |
There was a problem hiding this comment.
In case we decide to land this PR before we have support for remote feature toggles: Do not collect metrics in production.
This seems counterintuitive because production is exactly where we'd like to monitor performance. But, I think we ought to first see this in action in a test environment.
| // This 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. |
There was a problem hiding this comment.
This will make more sense once the feature toggles will be configurable remotely 😅
|
@jkmassel @crazytonyli despite this being in draft, I'd appreciate your input on the implementation details that are unrelated to where the One thing that I'm unsure at the moment, however, is the timing of the initialization: Do we wait to instantiate the Tracks stack till we have fetched the feature flags? Or, should we implement a way to update the configuration once new flags are received? |
You can test the changes from this Pull Request by:
|
mokagio
force-pushed
the
mokagio/performance-monitoring
branch
from
37198fa to
1d1f3a2
Compare
| // FIXME: Is there a way to control this via feature flags? | ||
| sampler: { 0.1 }, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😿
There was a problem hiding this comment.
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.
mokagio
force-pushed
the
mokagio/performance-monitoring
branch
from
1d1f3a2 to
9d3a6d4
Compare
mokagio
changed the title
| // FIXME: Is there a way to control this via feature flags? | ||
| sampler: { 0.1 }, |
There was a problem hiding this comment.
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 😿
mokagio
force-pushed
the
mokagio/performance-monitoring
branch
from
9d3a6d4 to
02d928c
Compare
Description
This PR aims to enable performance monitoring, the concrete implementation of which lives in Tracks and is currently implemented via Sentry, behind feature flags.
When I started working on this, I was under the impression WooCommerce iOS already had support for remote feature flags, but that doesn't seem to be the case. I must have applied my iOS bias to the conversations I had about remote feature flags, not realizing they were about the Android app.
I'm leaving this PR here to pick back up once we'll have support for remote feature flag. Or once someone tells me I got it wrong and it was there all along.I used the 11.3 milestone for this PR. It's the future-most milestone at the time of creating the PR.Update: Speaking with @jkmassel, we decided to get this PR to a ready-to-merge state, where all the logic is available but off, ready to build on top of once we'll be ready to tap into the remote feature toggles.
Testing instructions
Download the installable build, then visit the Sentry page to verify performance events from your device have been received.
If instead you run the app from Xcode, you should see no events.
RELEASE-NOTES.txtif necessary.