damondouglas commented on code in PR #24957:
URL: https://github.com/apache/beam/pull/24957#discussion_r1130147671
##########
learning/tour-of-beam/frontend/lib/locator.dart:
##########
@@ -61,3 +64,12 @@ void _initializeState() {
),
);
}
+
+void _initializeServices() {
+ final googleAnalyticsService = GoogleAnalytics4Service(
+ propertyId: kAnalyticsUA,
+ );
+ GetIt.instance.registerSingleton<AnalyticsService>(
+ googleAnalyticsService,
+ );
Review Comment:
Instead of using GetIt why not just reference googleAnalyticsService? The
project claims
> As your App grows, at some point you will need to put your app's logic in
classes that are separated from your Widgets. Keeping your widgets from having
direct dependencies makes your code better organized and easier to test and
maintain.
It isn't clear from this what is actually implementing the abstract
AnalyticsService class without some digging. The promise to make the code
better organized, at least in my attempt at reading the code, didn't seem to
happen.
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/share_tabs.dart:
##########
@@ -27,11 +27,13 @@ import 'snippet_save_and_share_tabs.dart';
/// content widget accordingly.
// TODO(alexeyinkin): Refactor code sharing,
https://github.com/apache/beam/issues/24637
class ShareTabs extends StatelessWidget {
+ final EventSnippetContext eventSnippetContext;
final VoidCallback onError;
final TabController tabController;
const ShareTabs({
super.key,
+ required this.eventSnippetContext,
Review Comment:
What is an eventSnippetContext? Could we see a code comment?
##########
playground/frontend/playground_components/lib/src/controllers/window_close_notifier/window_close_notifier_web.dart:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:html'; // ignore: avoid_web_libraries_in_flutter
+
+import 'package:flutter/foundation.dart';
+
+/// Notifies when the browser window is being closed.
+class WindowCloseNotifier extends ChangeNotifier {
Review Comment:
It isn't clear why this class is needed. Quick search through this PR, I
only see adding and removing of listeners but no code in response to this
event. Is it above with
`PlaygroundComponents.analyticsService.sendUnawaited(LoadedAnalyticsEvent`? In
other words are we wanting to analyze when the user closed the window and ended
their interaction with the application?
##########
learning/tour-of-beam/frontend/lib/models/event_context.dart:
##########
@@ -18,27 +18,31 @@
import 'package:equatable/equatable.dart';
-class AnalyticsEvent with EquatableMixin {
- final String action;
- final String category;
- final String? label;
- final Map<String, String>? parameters;
- final int? value;
+const _none = 'none';
- AnalyticsEvent({
- required this.action,
- required this.category,
- this.label,
- this.parameters,
- this.value,
+/// Basic information of the Tour of Beam state to augment analytics events.
+class TobEventContext with EquatableMixin {
+ const TobEventContext({
+ required this.sdkId,
+ required this.unitId,
});
+ final String? sdkId;
+ final String? unitId;
Review Comment:
From a code design perspective, why are these nullable when required in the
constructor?
##########
playground/frontend/playground_components/lib/src/services/analytics/google_analytics4_service/google_analytics4_service_web.dart:
##########
@@ -0,0 +1,102 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+import 'dart:convert';
+import 'dart:html'; // ignore: avoid_web_libraries_in_flutter
+import 'dart:js' as js; // ignore: avoid_web_libraries_in_flutter
+
+import 'package:meta/meta.dart';
+
+import '../events/abstract.dart';
+import 'google_analytics4_service.dart';
+
+final _urlTemplate = Uri.parse('https://www.googletagmanager.com/gtag/js');
+const _eventNameParam = 'eventName';
+
+/// The global JS function to submit analytics data.
+const _function = 'gtag';
+
+@internal
+GoogleAnalytics4Service createGoogleAnalytics4Service({
+ required String propertyId,
+}) =>
+ GoogleAnalytics4ServiceWeb(propertyId: propertyId);
+
+/// Submits data to a Google Analytics 4 property using JavaScript.
+class GoogleAnalytics4ServiceWeb extends GoogleAnalytics4Service {
Review Comment:
Can we see tests for this class? I see on the GetIt README, the author
writes, "I missed the ability to easily switch the implementation for a mocked
version without changing the UI". It seems to suggest that using GetIt seems
to provide the ability, as the author writes on the README, "make tests easier".
##########
learning/tour-of-beam/frontend/lib/models/event_context.dart:
##########
@@ -18,27 +18,31 @@
import 'package:equatable/equatable.dart';
-class AnalyticsEvent with EquatableMixin {
- final String action;
- final String category;
- final String? label;
- final Map<String, String>? parameters;
- final int? value;
+const _none = 'none';
- AnalyticsEvent({
- required this.action,
- required this.category,
- this.label,
- this.parameters,
- this.value,
+/// Basic information of the Tour of Beam state to augment analytics events.
+class TobEventContext with EquatableMixin {
+ const TobEventContext({
+ required this.sdkId,
+ required this.unitId,
});
+ final String? sdkId;
+ final String? unitId;
+
+ static const empty = TobEventContext(
+ sdkId: null,
+ unitId: null,
+ );
Review Comment:
Could we spell out Tob to TourOfBeam and it isn't clear why it needs to
implement EquatableMixin? Is it because there are two nullable properties and
doing equals on this is challenging?
##########
learning/tour-of-beam/frontend/lib/locator.dart:
##########
@@ -61,3 +64,12 @@ void _initializeState() {
),
);
}
+
+void _initializeServices() {
+ final googleAnalyticsService = GoogleAnalytics4Service(
Review Comment:
What if the analytics service changes again from GoogleAnalytics4Service to
something else?
##########
learning/tour-of-beam/frontend/lib/services/analytics/events/unit_opened.dart:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'abstract.dart';
+import 'constants.dart';
+
+class UnitOpenedTobAnalyticsEvent extends AnalyticsEventWithTobContext {
Review Comment:
Could we see a code comment explaining its purpose?
##########
playground/frontend/lib/modules/editor/components/share_dropdown/share_tabs/share_format_enum.dart:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
Review Comment:
I realize that the enum is obvious from its members but could we see a code
comment describing its purpose which I'm assuming is in the context of
Analytics events?
##########
playground/frontend/playground_components/lib/playground_components.dart:
##########
@@ -40,20 +47,40 @@ export 'src/models/example_view_options.dart';
export 'src/models/intents.dart';
export 'src/models/loading_status.dart';
export 'src/models/outputs.dart';
+export 'src/models/run_shortcut.dart';
export 'src/models/sdk.dart';
export 'src/models/shortcut.dart';
export 'src/models/snippet_file.dart';
export 'src/models/toast.dart';
export 'src/models/toast_type.dart';
+
export 'src/playground_components.dart';
+
export 'src/repositories/code_client/grpc_code_client.dart';
export 'src/repositories/code_repository.dart';
export 'src/repositories/example_client/grpc_example_client.dart';
export 'src/repositories/example_repository.dart';
+
export 'src/router/router_delegate.dart';
+
+export 'src/services/analytics/analytics_service.dart';
+export 'src/services/analytics/events/abstract.dart';
+export 'src/services/analytics/events/app_rated.dart';
+export 'src/services/analytics/events/constants.dart';
+export 'src/services/analytics/events/external_url_navigated.dart';
+export 'src/services/analytics/events/feedback_form_sent.dart';
+export 'src/services/analytics/events/report_issue_clicked.dart';
+export 'src/services/analytics/events/run_cancelled.dart';
+export 'src/services/analytics/events/run_finished.dart';
+export 'src/services/analytics/events/run_started.dart';
+export 'src/services/analytics/events/sdk_selected.dart';
+export 'src/services/analytics/events/snippet_reset.dart';
+export
'src/services/analytics/google_analytics4_service/google_analytics4_service.dart';
Review Comment:
Wouldn't this make sense to be part of a `playground_services` library
instead of a `playground_components` library?
##########
learning/tour-of-beam/frontend/lib/config.dart:
##########
@@ -26,14 +26,10 @@ const cloudFunctionsBaseUrl = 'https://'
// Copied from Playground's config.g.dart
-const String kAnalyticsUA = 'UA-73650088-2';
-const String kApiClientURL =
- 'https://backend-router-beta-dot-apache-beam-testing.appspot.com';
-const String kApiJavaClientURL =
- 'https://backend-java-beta-dot-apache-beam-testing.appspot.com';
-const String kApiGoClientURL =
- 'https://backend-go-beta-dot-apache-beam-testing.appspot.com';
-const String kApiPythonClientURL =
- 'https://backend-python-beta-dot-apache-beam-testing.appspot.com';
-const String kApiScioClientURL =
- 'https://backend-scio-beta-dot-apache-beam-testing.appspot.com';
+const String kAnalyticsUA = 'G-BXFP2FNCKC';
+
+const String kApiClientURL = 'https://router.play-dev.beam.apache.org';
+const String kApiJavaClientURL = 'https://java.play-dev.beam.apache.org';
+const String kApiGoClientURL = 'https://go.play-dev.beam.apache.org';
+const String kApiPythonClientURL = 'https://python.play-dev.beam.apache.org';
+const String kApiScioClientURL = 'https://scio.play-dev.beam.apache.org';
Review Comment:
Code comment where these come from and how they are derived? Someone should
read this and know how to change if needed.
##########
playground/frontend/lib/services/analytics/events/constants.dart:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class PlaygroundAnalyticsEvents {
+ static const loaded = 'loaded';
+ static const newExample = 'new_example';
+ static const shareableCopied = 'shareable_copied';
+ static const shareCodeClicked = 'share_code_clicked';
+ static const shortcutsClicked = 'shortcuts_clicked';
+ static const snippetSelected = 'snippet_selected';
+}
+
+class PlaygroundEventParams {
+ static const shareFormat = 'shareFormat';
Review Comment:
Why is there a class with a single static const property?
##########
playground/frontend/lib/pages/enum.dart:
##########
@@ -0,0 +1,22 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+enum PagesEnum {
+ embeddedPlayground,
+ standalonePlayground,
+}
Review Comment:
May we see a code comment describing the purpose of this enum and its
members, perhaps in the context of analytics?
##########
playground/frontend/lib/services/analytics/events/constants.dart:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class PlaygroundAnalyticsEvents {
Review Comment:
Comment
##########
learning/tour-of-beam/frontend/lib/config.dart:
##########
@@ -26,14 +26,10 @@ const cloudFunctionsBaseUrl = 'https://'
// Copied from Playground's config.g.dart
-const String kAnalyticsUA = 'UA-73650088-2';
-const String kApiClientURL =
- 'https://backend-router-beta-dot-apache-beam-testing.appspot.com';
-const String kApiJavaClientURL =
- 'https://backend-java-beta-dot-apache-beam-testing.appspot.com';
-const String kApiGoClientURL =
- 'https://backend-go-beta-dot-apache-beam-testing.appspot.com';
-const String kApiPythonClientURL =
- 'https://backend-python-beta-dot-apache-beam-testing.appspot.com';
-const String kApiScioClientURL =
- 'https://backend-scio-beta-dot-apache-beam-testing.appspot.com';
+const String kAnalyticsUA = 'G-BXFP2FNCKC';
Review Comment:
Why is this value here and also configured in the gradle?
##########
playground/frontend/playground_components/lib/src/models/event_snippet_context.dart:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:equatable/equatable.dart';
+
+import 'sdk.dart';
+
+const _none = 'none';
+
+/// Basic information of the Playground state at a particular moment of time
+/// to augment analytics events.
+class EventSnippetContext with EquatableMixin {
Review Comment:
Why does this need to implement EquatableMixin mixin?
##########
playground/frontend/playground_components/lib/src/enums/feedback_rating.dart:
##########
@@ -0,0 +1,22 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+enum FeedbackRating {
+ positive,
+ negative,
+}
Review Comment:
Is this being sent to the analytics service?
##########
playground/frontend/playground_components/lib/src/models/event_snippet_context.dart:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:equatable/equatable.dart';
+
+import 'sdk.dart';
+
+const _none = 'none';
+
+/// Basic information of the Playground state at a particular moment of time
+/// to augment analytics events.
+class EventSnippetContext with EquatableMixin {
+ const EventSnippetContext({
+ required this.originalSnippet,
+ required this.sdk,
+ required this.snippet,
+ });
+
+ /// Any identifier of the snippet that the content was derived from.
+ ///
+ /// If the code was modified it is still the last snippet loaded or chosen
+ /// by the user.
+ final String? originalSnippet;
+
+ final Sdk? sdk;
+
+ /// Any identifier of the current snippet if it is unchanged.
+ final String? snippet;
Review Comment:
From a code design perspective, why was it decided to make these nullable
but required in the constructor?
##########
playground/frontend/lib/services/analytics/events/loaded.dart:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:playground_components/playground_components.dart';
+
+import 'constants.dart';
+
+/// Playground is loaded.
+class LoadedAnalyticsEvent extends AnalyticsEvent {
Review Comment:
When you change this PR to add more tests, it would be nice to see this
included. It looks like this and its base AnalyticsEvent class are just value
classes without any exciting functionality so perhaps we can see coverage in
the context of other analytics tests such as a test that loads the Playground
and asserts that the LoadedAnalyticsEvent has the assigned sdk and/or snippet.
##########
learning/tour-of-beam/frontend/lib/pages/tour/controllers/unit.dart:
##########
@@ -18,28 +18,40 @@
import 'package:flutter/widgets.dart';
import 'package:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
import '../../../cache/unit_progress.dart';
+import '../../../models/event_context.dart';
+import '../../../models/unit.dart';
import '../../../repositories/client/client.dart';
+import '../../../services/analytics/events/unit_completed.dart';
class UnitController extends ChangeNotifier {
- final String unitId;
- final String sdkId;
+ final UnitModel unit;
Review Comment:
What is a UnitModel? Is this the model class representing the unit? Should
this be documented here as a code comment? Also to improve code readability,
could we prefix all classes coming from outside with the library it is imported
from? For example, I would have to open each imported file here to find where
UnitModel comes from or do a search for its symbol.
##########
learning/tour-of-beam/frontend/lib/models/event_context.dart:
##########
@@ -18,27 +18,31 @@
import 'package:equatable/equatable.dart';
-class AnalyticsEvent with EquatableMixin {
- final String action;
- final String category;
- final String? label;
- final Map<String, String>? parameters;
- final int? value;
+const _none = 'none';
- AnalyticsEvent({
- required this.action,
- required this.category,
- this.label,
- this.parameters,
- this.value,
+/// Basic information of the Tour of Beam state to augment analytics events.
Review Comment:
What Tour of Beam state? The SdkId and the UnitId?
##########
playground/frontend/lib/services/analytics/events/new_example.dart:
##########
@@ -0,0 +1,29 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'package:playground_components/playground_components.dart';
+
+import 'constants.dart';
+
+/// 'New Example' button is clicked in the app header.
+class NewExampleAnalyticsEvent extends AnalyticsEvent {
Review Comment:
It would be great to see more test coverage for this. Like other extensions
of AnalyticsEvent seemingly just data classes, it would be great to assert that
the values are what is expected (I see the name property for example). In this
context I would like to see a test mocking New Example button clicked and that
the assertion passes that the name is what it should be.
##########
playground/frontend/playground_components/lib/src/services/analytics/analytics_service.dart:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+import 'dart:collection';
+
+import 'package:get_it/get_it.dart';
+import 'package:meta/meta.dart';
+
+import 'events/abstract.dart';
+
+/// Abstract class to submit analytics events.
+abstract class AnalyticsService {
+ AnalyticsEvent? _lastEvent;
+ AnalyticsEvent? get lastEvent => _lastEvent;
+
+ final _defaultEventParameters = <String, dynamic>{};
+
+ /// The parameters sent with all events.
+ ///
+ /// Individual event parameters have higher priority on collisions.
+ Map<String, dynamic> get defaultEventParameters =>
+ UnmodifiableMapView(_defaultEventParameters);
+
+ set defaultEventParameters(Map<String, dynamic> newValue) {
+ _defaultEventParameters.clear();
+ _defaultEventParameters.addAll(newValue);
+ }
+
+ static AnalyticsService get() {
+ return GetIt.instance.get<AnalyticsService>();
+ }
+
+ /// Sends [event] asynchronously without returning a [Future].
+ void sendUnawaited(AnalyticsEvent event) {
Review Comment:
Why can't we just have `Future<void> send(AnalyticsEvent event) async { ...
}`?
##########
learning/tour-of-beam/frontend/lib/models/event_context.dart:
##########
@@ -18,27 +18,31 @@
import 'package:equatable/equatable.dart';
-class AnalyticsEvent with EquatableMixin {
- final String action;
- final String category;
- final String? label;
- final Map<String, String>? parameters;
- final int? value;
+const _none = 'none';
- AnalyticsEvent({
- required this.action,
- required this.category,
- this.label,
- this.parameters,
- this.value,
+/// Basic information of the Tour of Beam state to augment analytics events.
+class TobEventContext with EquatableMixin {
Review Comment:
Could we spell out TourOfBeam instead of using the abbreviation?
##########
playground/frontend/playground_components/lib/src/services/analytics/analytics_service.dart:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import 'dart:async';
+import 'dart:collection';
+
+import 'package:get_it/get_it.dart';
+import 'package:meta/meta.dart';
+
+import 'events/abstract.dart';
+
+/// Abstract class to submit analytics events.
+abstract class AnalyticsService {
+ AnalyticsEvent? _lastEvent;
+ AnalyticsEvent? get lastEvent => _lastEvent;
+
+ final _defaultEventParameters = <String, dynamic>{};
+
+ /// The parameters sent with all events.
+ ///
+ /// Individual event parameters have higher priority on collisions.
+ Map<String, dynamic> get defaultEventParameters =>
+ UnmodifiableMapView(_defaultEventParameters);
+
+ set defaultEventParameters(Map<String, dynamic> newValue) {
+ _defaultEventParameters.clear();
+ _defaultEventParameters.addAll(newValue);
+ }
+
+ static AnalyticsService get() {
+ return GetIt.instance.get<AnalyticsService>();
Review Comment:
I don't see how GetIt is making it clear where the actual implementation
exists. I had to search quite a bit until I found the actual implementation.
Nonetheless, if we decide to keep and use GetIt, can we make good use of it and
expand our test coverage as the package promises to make easier?
##########
playground/frontend/lib/modules/actions/components/new_example_action.dart:
##########
@@ -38,7 +38,9 @@ class NewExampleAction extends StatelessWidget {
label: 'intents.playground.newExample'.tr(),
onPressed: () {
launchUrl(Uri.parse('/'));
- AnalyticsService.get(context).trackClickNewExample();
+ PlaygroundComponents.analyticsService.sendUnawaited(
Review Comment:
I agree with @Malarg here that sendNewExampleEvent makes sense instead of
sendUnawaited.
##########
learning/tour-of-beam/frontend/lib/locator.dart:
##########
@@ -61,3 +64,12 @@ void _initializeState() {
),
);
}
+
+void _initializeServices() {
+ final googleAnalyticsService = GoogleAnalytics4Service(
Review Comment:
Also, is it possible to alias the library imports so someone knows where the
class is from? So instead of `SomeClass()`, we have `library.SomeClass()`?
Yes, in an IDE one could navigate to definition but many times people are
reading code on GitHub directly; it's symbol lookup doesn't always work.
##########
playground/frontend/lib/components/playground_run_or_cancel_button.dart:
##########
@@ -20,38 +20,38 @@ import 'package:flutter/widgets.dart';
import 'package:playground_components/playground_components.dart';
import 'package:provider/provider.dart';
-import '../modules/analytics/analytics_service.dart';
-import '../utils/analytics_utils.dart';
-
class PlaygroundRunOrCancelButton extends StatelessWidget {
const PlaygroundRunOrCancelButton();
@override
Widget build(BuildContext context) {
return Consumer<PlaygroundController>(
- builder: (context, playgroundController, child) {
- final analyticsService = AnalyticsService.get(context);
- final stopwatch = Stopwatch();
- final exampleName = getAnalyticsExampleName(playgroundController);
-
- return RunOrCancelButton(
- playgroundController: playgroundController,
- beforeCancel: () {
- final exampleName = getAnalyticsExampleName(playgroundController);
- analyticsService.trackClickCancelRunEvent(exampleName);
- },
- beforeRun: () {
- stopwatch.start();
- analyticsService.trackClickRunEvent(exampleName);
- },
- onComplete: () {
- analyticsService.trackRunTimeEvent(
- exampleName,
- stopwatch.elapsedMilliseconds,
- );
- },
- );
- }
+ builder: (context, playgroundController, child) => RunOrCancelButton(
+ playgroundController: playgroundController,
+ beforeCancel: (runner) {
+ PlaygroundComponents.analyticsService.sendUnawaited(
Review Comment:
I agree with @Malarg that sendCancelRun makes a little more sense than
sendUnawaited but is still unclear to me. Should it be called sendRunOrCancel
to be consistent with the RunOrCancelButton?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
