Skip to content

Commit 933f5ea

Browse files
authored
[unified_analytics] Send enabled_features as an event parameter rather than a user property (#2007)
Fixes flutter/flutter#147327. **Summary.** The `Analytics` constructor accepts a list of "enabled features" for the current user of the whatever tool is in use ([src](https://github.com/dart-lang/tools/blob/3934675f1595ed2fee2297d7d1cc96beaaa50c9e/pkgs/unified_analytics/lib/src/analytics.dart#L39-L42)). For example, in the Flutter tool, this will be a comma-delimited list of `flutter config` items set to `true` (explicitly by the user). When any event is sent off to Google Analytics, this list will be included as a _user property_ with the key "enabled_features". See the parent issue. This list is prone to getting truncated due to the low character limit of user property values. This PR instead sends them as event properties. This has a _much_ higher character limit of 500, which is hopefully sufficient (even if it is not, it would be a huge improvement over the current limit of 36, which makes collecting enabled features near-useless). However, this creates a pitfall. If any new `Event` decides to include an `enabled_features` event property (and also passes a non-null argument for the `enabledFeatures` parameter in the `Analytics` constructor). There are a few ways handle this: 1. Do nothing. The likelihood of this happening is extremely low (but it will be quite confusing if it does happen). 2. Create an `Event._` constructor that throws an exception if the `eventData` map includes an entry with a key of `enabled_features`. Re-route all of the existing constructors to utilize this one. This working depends on newly added events following the pattern of using `Event._` instead of doing initialization themselves; but I suspect folks adding new events will naturally use the existing ones for inspiration, so this is fine to me. 3. Similar to 2, add this validation logic but instead do so in `generateRequestBody`. This keeps the validation logic much closer to the logic that inserts `enabled_features`, so it will make sense to first-time maintainers slightly more quickly. This also would require the least work to implement. The downside is that no exception would appear until `send` is called, and that exception would be thrown asynchronously. I suspect 2 is probably the way to go, so I went ahead and implemented that. Let me know if you have a differing opinion. I made this is a separate commit, so it would be easy to revert. --- <details> <summary>Contribution guidelines:</summary><br> - See our [contributor guide](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md) for general expectations for PRs. - Larger or significant changes should be discussed in an issue before creating a PR. - Contributions to our repos should follow the [Dart style guide](https://dart.dev/guides/language/effective-dart) and use `dart format`. - Most changes should add an entry to the changelog and may need to [rev the pubspec package version](https://github.com/dart-lang/sdk/blob/main/docs/External-Package-Maintenance.md#making-a-change). - Changes to packages require [corresponding tests](https://github.com/dart-lang/.github/blob/main/CONTRIBUTING.md#Testing). Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback. </details>
1 parent ce87a2b commit 933f5ea

File tree

8 files changed

+368
-274
lines changed

8 files changed

+368
-274
lines changed

pkgs/unified_analytics/CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
## 7.0.2
2+
- Send `enabled_features` as an event parameter in all events rather than as a user property.
3+
14
## 7.0.1
25
- Fixed `UnsupportedError` thrown when Event.exception is called without providing a value for `args`.
36

pkgs/unified_analytics/lib/src/analytics.dart

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,7 @@ class AnalyticsImpl implements Analytics {
339339
final File _clientIdFile;
340340
final UserProperty _userProperty;
341341
final LogHandler _logHandler;
342+
final String? _enabledFeatures;
342343

343344
/// Tells the client if they need to show a message to the
344345
/// user; this will return true if it is the first time the
@@ -406,8 +407,8 @@ class AnalyticsImpl implements Analytics {
406407
truncateStringToLength(io.Platform.operatingSystemVersion, 36),
407408
locale: io.Platform.localeName,
408409
clientIde: clientIde,
409-
enabledFeatures: enabledFeatures,
410410
),
411+
_enabledFeatures = enabledFeatures,
411412
_configHandler = ConfigHandler(
412413
homeDirectory: homeDirectory,
413414
configFile: homeDirectory
@@ -613,6 +614,7 @@ class AnalyticsImpl implements Analytics {
613614
eventName: event.eventName,
614615
eventData: event.eventData,
615616
userProperty: _userProperty,
617+
enabledFeatures: _enabledFeatures,
616618
);
617619

618620
if (_enableAsserts) checkBody(body);
@@ -654,6 +656,7 @@ class AnalyticsImpl implements Analytics {
654656
eventName: collectionEvent.eventName,
655657
eventData: collectionEvent.eventData,
656658
userProperty: _userProperty,
659+
enabledFeatures: _enabledFeatures,
657660
);
658661

659662
_logHandler.save(data: body);
@@ -664,6 +667,7 @@ class AnalyticsImpl implements Analytics {
664667
clientId: clientId,
665668
eventName: collectionEvent.eventName,
666669
eventData: collectionEvent.eventData,
670+
enabledFeatures: _enabledFeatures,
667671
userProperty: _userProperty,
668672
);
669673

@@ -774,6 +778,7 @@ class FakeAnalytics extends AnalyticsImpl {
774778
eventName: event.eventName,
775779
eventData: event.eventData,
776780
userProperty: _userProperty,
781+
enabledFeatures: _enabledFeatures,
777782
);
778783

779784
if (_enableAsserts) checkBody(body);

pkgs/unified_analytics/lib/src/constants.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ const int kMaxLogFileSize = 25 * (1 << 20);
8787
const String kLogFileName = 'dart-flutter-telemetry.log';
8888

8989
/// The current version of the package, should be in line with pubspec version.
90-
const String kPackageVersion = '7.0.1';
90+
const String kPackageVersion = '7.0.2';
9191

9292
/// The minimum length for a session.
9393
const int kSessionDurationMinutes = 30;

0 commit comments

Comments
 (0)