alexeyinkin commented on code in PR #24957:
URL: https://github.com/apache/beam/pull/24957#discussion_r1087443932
##########
learning/tour-of-beam/frontend/lib/pages/tour/state.dart:
##########
@@ -57,6 +62,7 @@ class TourNotifier extends ChangeNotifier with
PageStateMixin<void> {
_appNotifier.addListener(_onAppNotifierChanged);
_authNotifier.addListener(_onUnitProgressChanged);
_onUnitChanged();
+ window.onBeforeUnload.listen(_onTabClosed);
Review Comment:
Make a `CloseNotifier` and do not import `dart:html` here?
##########
learning/tour-of-beam/frontend/lib/modules/analytics/google_analytics_service.dart:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:usage/usage_html.dart';
Review Comment:
Is there a way to use a platform-agnostic implementation?
##########
learning/tour-of-beam/frontend/lib/modules/analytics/google_analytics_service.dart:
##########
@@ -0,0 +1,130 @@
+/*
+ * 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:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+import 'package:usage/usage_html.dart';
+
+import '../../config.dart';
+import '../../models/module.dart';
+import '../../models/unit.dart';
+import 'categories.dart';
+import 'events.dart';
+import 'service.dart';
+
+class TobGoogleAnalyticsService extends GoogleAnalyticsService
+ implements TobAnalyticsService {
+ static const _appName = 'beam';
+ static const _appVersion = '1.0';
+ final _analytics = AnalyticsHtml(kAnalyticsUA, _appName, _appVersion);
+
+ static TobGoogleAnalyticsService get() {
+ return GetIt.instance.get<TobGoogleAnalyticsService>();
+ }
+
+ TobGoogleAnalyticsService()
+ : super(
+ appAnalytics: AnalyticsHtml(
+ kAnalyticsUA,
+ _appName,
+ _appVersion,
+ ),
+ );
+
+ @override
+ Future<void> openUnit(Sdk sdk, UnitModel unit) async {
+ await _safeSendEvent(
+ AnalyticsEvent(
+ action: TobAnalyticsEvents.openUnit,
+ category: TobAnalyticsCategories.unit,
+ label: '${sdk.title}_${unit.id}',
+ ),
+ );
+ }
+
+ @override
+ Future<void> closeUnit(Sdk sdk, String unitId, Duration timeSpent) async {
+ await _safeSendEvent(
+ AnalyticsEvent(
+ action: TobAnalyticsEvents.closeUnit,
+ category: TobAnalyticsCategories.unit,
+ label: '${sdk.title}_${unitId}_${timeSpent.inSeconds}s',
Review Comment:
Is there a way to send raw property values for `sdk.title`, `unit.id`, and
seconds to filter the events later?
##########
learning/tour-of-beam/frontend/lib/modules/analytics/service.dart:
##########
@@ -0,0 +1,37 @@
+/*
+ * 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:get_it/get_it.dart';
+import 'package:playground_components/playground_components.dart';
+
+import '../../models/module.dart';
+import '../../models/unit.dart';
+
+abstract class TobAnalyticsService extends AnalyticsService {
+ static TobAnalyticsService get() {
+ return GetIt.instance.get<TobAnalyticsService>();
+ }
+
+ Future<void> openUnit(Sdk sdk, UnitModel unit);
+ Future<void> closeUnit(Sdk sdk, String unitId, Duration timeSpent);
+ Future<void> completeUnit(Sdk sdk, UnitModel unit);
+ // TODO(nausharipov): implement
Review Comment:
?
##########
playground/frontend/lib/config.g.dart:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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:
- We are not yet allowed to commit this file. See
https://github.com/apache/beam/pull/25022
##########
learning/tour-of-beam/frontend/lib/config.dart:
##########
@@ -26,7 +26,8 @@ const cloudFunctionsBaseUrl = 'https://'
// Copied from Playground's config.g.dart
-const String kAnalyticsUA = 'UA-73650088-2';
+const String kAnalyticsUA = 'G-349612187';
Review Comment:
Does the `usage` package work with the Analytics-4 new counters?
##########
playground/frontend/lib/modules/analytics/categories.dart:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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 IND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class AnalyticsCategories {
+ static const common = 'Common';
+ // TODO(nausharipov): runCode and example are duplicated in PGC
Review Comment:
Then can we remove them from here? Same with events like `click_cancel_run`
etc.
##########
playground/frontend/playground_components/lib/src/services/analytics/generic/categories.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.
+ */
+
+class GenericAnalyticsCategories {
Review Comment:
```suggestion
class BeamAnalyticsCategories {
```
?
##########
playground/frontend/lib/pages/standalone_playground/widgets/feedback/rating_enum.dart:
##########
@@ -0,0 +1,19 @@
+/*
+ * 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:
Trailing comma, auto-format.
##########
playground/frontend/playground_components/lib/src/services/analytics/google_analytics_service.dart:
##########
@@ -0,0 +1,75 @@
+/*
+ * 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:usage/usage_html.dart';
+
+import 'event.dart';
+import 'generic/categories.dart';
+import 'generic/events.dart';
+import 'service.dart';
+
+class GoogleAnalyticsService implements AnalyticsService {
+ late AnalyticsHtml _appAnalytics;
+
+ GoogleAnalyticsService({
+ required AnalyticsHtml appAnalytics,
+ }) {
+ _appAnalytics = appAnalytics;
+ }
+
+ @override
+ AnalyticsEvent? lastSentEvent;
+
Review Comment:
There should be more events common for the two projects. Cancelling comes to
mind.
I suggest also adding feedback here. This way we make sure that both ToB and
Playground handle the feedback the same way.
##########
playground/frontend/lib/pages/standalone_playground/widgets/feedback/playground_feedback.dart:
##########
@@ -67,7 +63,7 @@ class PlaygroundFeedback extends StatelessWidget {
_setEnjoying(BuildContext context, bool isEnjoying) {
return () {
_getFeedbackState(context, false).setEnjoying(isEnjoying);
- AnalyticsService.get(context).trackClickEnjoyPlayground(isEnjoying);
+ PlaygroundAnalyticsService.get().trackClickEnjoyPlayground(isEnjoying);
Review Comment:
Use `FeedbackRating` everywhere instead of bool?
##########
playground/frontend/playground_components/lib/src/services/analytics/generic/events.dart:
##########
@@ -0,0 +1,23 @@
+/*
+ * 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 GenericAnalyticsEvents {
+ static const run = 'click_run';
+ // TODO(nausharipov): rename example to snippet everywhere?
Review Comment:
I suggest keeping it to not break the continuity of statistics.
##########
playground/frontend/playground_components/lib/src/widgets/run_or_cancel_button.dart:
##########
@@ -49,7 +50,10 @@ class RunOrCancelButton extends StatelessWidget {
(_) => PlaygroundComponents.toastNotifier.add(_getErrorToast()),
);
},
- runCode: () {
+ runCode: () async {
+ await AnalyticsService.get().trackRunExample(
+ playgroundController.selectedExample!.name,
+ );
Review Comment:
Maybe not await most of the events? Except maybe feedback because the event
is the business logic there.
--
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]
