feat: add `analyticsKernelService` with renderer-protocol #3605

kuruk-mm · 2022-11-28T12:28:53Z

What does this PR change?

Use renderer-protocol instead of JSON-based communication

Test

Open https://play.decentraland.zone/?renderer-branch=feat/analytics&kernel-branch=feat/analytics
Check if PerformanceReport, SystemInfoReport, AnalyticsEvent, SetDelightedSurveyEnabled, works

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

github-actions · 2022-11-28T12:29:06Z

After the CI passes:

This branch can be previewed at https://play.decentraland.zone/?renderer-branch=feat/analytics&kernel-branch=feat/analytics
Code coverage report: https://renderer-artifacts.decentraland.org/branch-coverage/feat/analytics
Benchmark report: https://renderer-artifacts.decentraland.org/branch-benchmark/feat/analytics/index.html

unity-renderer/Assets/Scripts/MainScripts/DCL/Controllers/Scene/Main/Main.cs

kuruk-mm · 2022-12-02T18:08:14Z

unity-renderer/Assets/Scripts/MainScripts/DCL/Controllers/Scene/Main/Main.cs

-                // TODO: remove useBinaryTransform after ECS7 is fully in prod
-                UseBinaryTransform = true,
-            });
-
            // We trigger the Decentraland logic once everything is initialized.
            WebInterface.StartDecentraland();


This function, starts the engine calling the web directly. And from the Kernel side, starts everything, including the RPC.

So I had to move the rpc call after this StartDecentraland()

Signed-off-by: Mateo Miccino <mateomiccino@gmail.com>

AjimenezDCL · 2022-12-05T08:41:09Z

unity-renderer/Assets/Scripts/MainScripts/DCL/WorldRuntime/KernelCommunication/RPC/RPC.cs

+            // this event should be the last one to be executed after initialization
+            // it is used by the kernel to signal "EngineReady" or something like that
+            // to prevent race conditions like "SceneController is not an object",
+            // aka sending events before unity is ready


We should document all this setup process somewhere, what type of signasl are required between kernel/renderer to start decentraland.

Also bear in mind we are aiming to register KernelServices as IService in the ServiceLocator, this initialization will be moved (probably to a subsystem in Main or similar). In the meantime we can have it here.

i think this method lacks of cohesion. Why is it called StartRpc but does an analytics system reports saying that should be the last thing to be called? Perhaps a rename: ReportSystemInfoToAnalytics

lorux0 · 2022-12-15T17:42:48Z

unity-renderer/Assets/Scripts/MainScripts/DCL/ServiceProviders/Analytics/Analytics.cs

-    internal void SendToSegment(string eventName, Dictionary<string, string> data) { WebInterface.ReportAnalyticsEvent(eventName, data.Select(x => new WebInterface.AnalyticsPayload.Property(x.Key, x.Value)).ToArray()); }
+    internal void SendToSegment(string eventName, Dictionary<string, string> data)
+    {
+        ClientAnalyticsKernelService analytics = DCL.Environment.i.serviceLocator.Get<IRPC>().Analytics();


could you inject the IRPC dependency?

lorux0 · 2022-12-15T17:46:37Z

...s/Scripts/MainScripts/DCL/WorldRuntime/Debugging/Performance/PerformanceMetricsController.cs

            };

-            var result = JsonConvert.SerializeObject(performanceReportPayload);
-            WebInterface.SendPerformanceReport(result);
+            ClientAnalyticsKernelService analytics = Environment.i.serviceLocator.Get<IRPC>().Analytics();


could you inject IRPC dependency?

lorux0 · 2022-12-15T17:49:38Z

unity-renderer/Assets/Scripts/MainScripts/DCL/WorldRuntime/KernelCommunication/RPC/RPC.cs

+            // this event should be the last one to be executed after initialization
+            // it is used by the kernel to signal "EngineReady" or something like that
+            // to prevent race conditions like "SceneController is not an object",
+            // aka sending events before unity is ready


i think this method lacks of cohesion. Why is it called StartRpc but does an analytics system reports saying that should be the last thing to be called? Perhaps a rename: ReportSystemInfoToAnalytics

lorux0 · 2022-12-15T17:51:58Z

unity-renderer/Assets/Tutorial/Scripts/TutorialController.cs

@@ -210,7 +212,9 @@ public void SetTutorialDisabled()
            commonDataStore.isTutorialRunning.Set(false);
            tutorialView.tutorialMusicHandler.StopTutorialMusic();
            ShowTeacher3DModel(false);
-            WebInterface.SetDelightedSurveyEnabled(true);
+
+            ClientAnalyticsKernelService analytics = DCL.Environment.i.serviceLocator.Get<IRPC>().Analytics();


can you inject IRPC dependency?

lorux0 · 2022-12-15T17:52:18Z

unity-renderer/Assets/Tutorial/Scripts/TutorialController.cs

            };

-            WebInterface.ReportAnalyticsEvent("tutorial step started", properties);
+            ClientAnalyticsKernelService analytics = DCL.Environment.i.serviceLocator.Get<IRPC>().Analytics();


avoid singletons coupling if possible

Signed-off-by: Mateo Miccino <mateomiccino@gmail.com>

eordano · 2023-02-06T16:14:01Z

...s/Scripts/MainScripts/DCL/WorldRuntime/Debugging/Performance/PerformanceMetricsController.cs

+                Samples = encodedSamples,
+                FpsIsCapped = usingFPSCap,
+                HiccupsInThousandFrames = hiccupsInThousandFrames,
+                HiccupsTime = hiccupsTime,
+                TotalTime = totalTime,
+                GltfInProgress = gltfloading,
+                GltfFailed = gltffailed,
+                GltfCancelled = gltfcancelled,
+                GltfLoaded = gltfloaded,
+                AbInProgress = abloading,
+                AbFailed = abfailed,
+                AbCancelled = abcancelled,
+                AbLoaded = abloaded,
+                GltfTexturesLoaded = gltfTextures,
+                AbTexturesLoaded = abTextures,
+                PromiseTexturesLoaded = promiseTextures,
+                EnqueuedMessages = queuedMessages,
+                ProcessedMessages = processedMessages,
+                PlayerCount = playerCount,
+                LoadRadius = (int) loadRadius,
+                SceneScores = { scenesMemoryScore },
+                DrawCalls = (int) drawCalls!,
+                MemoryReserved = (long) totalMemoryReserved!,
+                MemoryUsage = (long) totalMemoryUsage,
+                TotalGcAlloc = (long) totalGCAlloc,


Wouldn't this case change involve a change in the behavior? Can we have the linter ignore these?

In theory, it should not change anything. It's sending with the new RPC instead of JSON.

lorux0 · 2023-03-17T15:35:50Z

.../DCL/DecentralandProtocol/Decentraland/renderer/kernel_services/AnalyticsService.gen.cs.meta

@@ -1,5 +1,9 @@
 fileFormatVersion: 2
+<<<<<<< HEAD:unity-renderer/Assets/Scripts/MainScripts/DCL/WorldRuntime/KernelCommunication/RPC/GeneratedCode/Decentraland/renderer/kernel_services/AnalyticsService.gen.cs.meta


wrong conflict solving

aixaCode · 2023-04-06T08:46:32Z

@kuruk-mm Is this still PR valid? What is the status with that?

kuruk-mm · 2023-05-17T10:54:22Z

Closing. Not more valid

kuruk-mm added 2 commits November 28, 2022 09:11

feat: analytics with renderer-protocol

148eb0b

add PerformanceReport

228e347

kuruk-mm added 6 commits November 28, 2022 09:29

Merge remote-tracking branch 'origin/dev' into feat/analytics

c27c78b

feat: ensure rpc

7fb1de5

move rpc server creation to the RPCService

13b5419

fix

a161654

Merge branch 'feat/ensure-rpc' into feat/analytics

e6e89e5

fixes

3923bcd

kuruk-mm changed the base branch from dev to feat/ensure-rpc November 29, 2022 13:14

kuruk-mm added 6 commits November 29, 2022 12:27

fixes

81ee491

Merge branch 'feat/ensure-rpc' into feat/analytics

085a5ed

minor fixes

4798054

fix

302d827

Merge remote-tracking branch 'origin/dev' into feat/ensure-rpc

e6b6b3d

Merge branch 'feat/ensure-rpc' into feat/analytics

4b254b1

kuruk-mm requested a review from AjimenezDCL November 30, 2022 15:47

improve readme

5aa67a8

kuruk-mm marked this pull request as ready for review December 1, 2022 01:46

github-actions bot requested a review from kwssbocian December 1, 2022 01:46

github-actions bot assigned kuruk-mm Dec 1, 2022

kuruk-mm removed the request for review from kwssbocian December 1, 2022 01:46

kuruk-mm added the No QA Needed Issues which do not require QA testing label Dec 1, 2022

kuruk-mm added 3 commits November 30, 2022 22:47

Merge branch 'feat/ensure-rpc' into feat/analytics

a3f5726

Merge branch 'dev' into feat/ensure-rpc

0035f4b

update protocol

44d9a78

kuruk-mm force-pushed the feat/analytics branch from 5e55efc to 44d9a78 Compare December 1, 2022 12:24

kuruk-mm added 2 commits December 1, 2022 09:52

improve readme

4ae6b47

Merge branch 'feat/ensure-rpc' into feat/analytics

0e3b6a8

mikhail-dcl reviewed Dec 1, 2022

View reviewed changes

unity-renderer/Assets/Scripts/MainScripts/DCL/Controllers/Scene/Main/Main.cs Outdated Show resolved Hide resolved

start rpc function

69d6780

kuruk-mm force-pushed the feat/analytics branch from 1af0c38 to 69d6780 Compare December 2, 2022 18:00

kuruk-mm commented Dec 2, 2022

View reviewed changes

kuruk-mm requested review from AjimenezDCL and mikhail-dcl December 2, 2022 18:08

kuruk-mm added 2 commits December 2, 2022 15:10

Merge branch 'dev' into feat/analytics

b7f8b39

Signed-off-by: Mateo Miccino <mateomiccino@gmail.com>

missing import

46ab319

kuruk-mm marked this pull request as ready for review December 2, 2022 18:12

AjimenezDCL approved these changes Dec 5, 2022

View reviewed changes

kuruk-mm added 3 commits December 12, 2022 15:00

Merge remote-tracking branch 'origin/dev' into feat/analytics

c7da753

Merge remote-tracking branch 'origin/dev' into feat/analytics

a3bb2c8

fixes

c7ca0e7

kuruk-mm force-pushed the feat/analytics branch from 22deca6 to c7ca0e7 Compare December 14, 2022 14:42

kuruk-mm added 2 commits December 14, 2022 17:41

Merge remote-tracking branch 'origin/dev' into feat/analytics

1c4c8fa

fix

f52b163

kuruk-mm requested a review from lorux0 December 14, 2022 20:47

lorux0 requested changes Dec 15, 2022

View reviewed changes

kuruk-mm added 2 commits December 21, 2022 19:27

Merge branch 'dev' into feat/analytics

d68349a

Merge branch 'dev' into feat/analytics

e90c87a

Signed-off-by: Mateo Miccino <mateomiccino@gmail.com>

eordano approved these changes Feb 6, 2023

View reviewed changes

eordano reviewed Feb 6, 2023

View reviewed changes

Merge branch 'dev' into feat/analytics

f3b4db5

eordano requested a review from a team as a code owner February 6, 2023 16:14

Merge branch 'dev' into feat/analytics

149a79f

kuruk-mm requested a review from lorux0 March 6, 2023 20:46

Merge branch 'dev' into feat/analytics

11b6fb1

lorux0 requested changes Mar 17, 2023

View reviewed changes

kuruk-mm closed this May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `analyticsKernelService` with renderer-protocol #3605

feat: add `analyticsKernelService` with renderer-protocol #3605

kuruk-mm commented Nov 28, 2022 •

edited

Loading

github-actions bot commented Nov 28, 2022 •

edited by kuruk-mm

Loading

kuruk-mm Dec 2, 2022

AjimenezDCL Dec 5, 2022

lorux0 Dec 15, 2022

lorux0 Dec 15, 2022

lorux0 Dec 15, 2022

lorux0 Dec 15, 2022

lorux0 Dec 15, 2022

lorux0 Dec 15, 2022

eordano Feb 6, 2023

kuruk-mm Mar 6, 2023

lorux0 Mar 17, 2023

aixaCode commented Apr 6, 2023

kuruk-mm commented May 17, 2023

		@@ -1,5 +1,9 @@
		fileFormatVersion: 2
		<<<<<<< HEAD:unity-renderer/Assets/Scripts/MainScripts/DCL/WorldRuntime/KernelCommunication/RPC/GeneratedCode/Decentraland/renderer/kernel_services/AnalyticsService.gen.cs.meta

feat: add analyticsKernelService with renderer-protocol #3605

feat: add analyticsKernelService with renderer-protocol #3605

Conversation

kuruk-mm commented Nov 28, 2022 • edited Loading

What does this PR change?

Test

Our Code Review Standards

github-actions bot commented Nov 28, 2022 • edited by kuruk-mm Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aixaCode commented Apr 6, 2023

kuruk-mm commented May 17, 2023

feat: add `analyticsKernelService` with renderer-protocol #3605

feat: add `analyticsKernelService` with renderer-protocol #3605

kuruk-mm commented Nov 28, 2022 •

edited

Loading

github-actions bot commented Nov 28, 2022 •

edited by kuruk-mm

Loading