Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix : Metrics - Remove all data collection from Stride.Engine #2261

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Jklawreszuk
Copy link
Collaborator

@Jklawreszuk Jklawreszuk commented May 21, 2024

PR Details

Description

Goodbye telemetry! 👋

Motivation and Context

Metrics feature in Stride.Engine can be considered as leftover from the time Xenko (now Stride Engine) was closed-source. While telemetry may be useful in commercial products, Stride Engine is being developed as free and open source software by independent volunteers.

Thus, information about current needs, bugs and errors is being frequently gathered by making requests on Github or directly through discussions with others on our Discord channel and there is no need to do any much more.

PR updates also Stride.Editor.CrashReporter. Previously, user could send anonymized report to support.stride3d.net, but this service has been forgotten and is useless since we are commonly use Github. Now it is possible to view and save the created report containing metrics that can be analyzed and - if the submitter wants it - attached to a new Github issue.

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My change requires a change to the documentation.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have built and run the editor to try this change out.

@IXLLEGACYIXL
Copy link
Collaborator

how does the crash report look where i was able to send an email to the non existent server with the crash report?

@Jklawreszuk
Copy link
Collaborator Author

@IXLLEGACYIXL Welp.. It's hard to tell if this server collects data at all. @xen2 have mentioned on discord a long time ago that it's closed-source (https://discord.com/channels/500285081265635328/500292370923913222/786936723283116062). About email : "Send" button just sends collection of key-value types and it doesn't matter whether you give your email or not

@Jklawreszuk
Copy link
Collaborator Author

Take a look at CrashReportData object

@Jklawreszuk Jklawreszuk marked this pull request as draft May 22, 2024 07:20
@Jklawreszuk Jklawreszuk force-pushed the metrics branch 2 times, most recently from 497109e to 1b3a56d Compare May 22, 2024 15:34
@Jklawreszuk Jklawreszuk marked this pull request as ready for review May 22, 2024 15:34
@Jklawreszuk
Copy link
Collaborator Author

My work is ready for review 😊

@Eideren
Copy link
Collaborator

Eideren commented May 25, 2024

I think we'll need @Kryptos-FR input on this given the changes to UI stuff. Also his input on using Modern.Forms when we have Avalonia coming in.

@Eideren Eideren requested a review from Kryptos-FR May 25, 2024 09:47
@Eideren
Copy link
Collaborator

Eideren commented May 26, 2024

Fixes #1663 I think ?
Related to #3 & #204 & #1815

@Kryptos-FR
Copy link
Member

Kryptos-FR commented May 30, 2024

using Modern.Forms when we have Avalonia coming in

If we can avoid adding new WPF/WinForms dependencies, that would be nice. What does this package bring that cannot already be done with standard controls?

@Kryptos-FR
Copy link
Member

In addition, did we check with a legal adviser whether removing the privacy policy is OK? We might stop collecting metrics directly, but we should keep the crash report that user can copy/paste into an issue and/or a discord chat. That report might contain user data for which we still need to comply with some laws.

@IXLLEGACYIXL
Copy link
Collaborator

IXLLEGACYIXL commented May 30, 2024

In addition, did we check with a legal adviser whether removing the privacy policy is OK? We might stop collecting metrics directly, but we should keep the crash report that user can copy/paste into an issue and/or a discord chat. That report might contain user data for which we still need to comply with some laws.

the crash report still exists but instead of mail and send report you have the open git issue button

@Kryptos-FR
Copy link
Member

Removing the metrics and removing the privacy policy should be two separate PRs in my opinion. I'm not convinced the privacy policy should go.

@Eideren
Copy link
Collaborator

Eideren commented May 30, 2024

I doubt there is any need for a privacy policy if users provide those information of their own volition, it would fall into github's own eula. On our end we just have to make it clear that users have to review what they send over and never automate the system.

@MeharDT
Copy link
Contributor

MeharDT commented May 30, 2024

IIRC the App Store and Google Play mandate a privacy policy for all apps, even those that don't collect any data. Using that as a reference it would still be good practise to continue including one, even if it effectively says "we don't collect anything". I don't know what jurisdiction Stride would fall under but it provides a benchmark for users and contributors to evaluate and develop against.

@Jklawreszuk
Copy link
Collaborator Author

I'm not a layer ofc but, it seems to me that the solution to our problem would be to make an announcement on stride website in the form of a blog and change the content of the "Privacy policy" page. Our current Privacy policy says:

If we make material changes to this Privacy Policy, we will notify you through the Software OR the web page that provided the Software to You(the “Site”) along with information on accessing the updated Privacy Policy.

Later:

Your continued use of the Software and the Services will signify your acceptance of the changes of our Privacy Policy.

@Jklawreszuk
Copy link
Collaborator Author

If we can avoid adding new WPF/WinForms dependencies, that would be nice. What does this package bring that cannot already be done with standard controls?

@Kryptos-FR I used the Modern.Forms package because it is a crossplatform reimplementation of WinForms and it was extremely easy to port the code, so I did it rather as a bonus. I don't mind rollbacking the changes though if you are going to rewrite this to avalonia 👍

@Kryptos-FR
Copy link
Member

@Jklawreszuk Modern.Forms is another dependency that we don't need since the cross-platform version will use Avalonia. On top of that their repo seems inactive for more than a year, and their own README indicates that it is not ready for production. We don't know what kind of bug it could bring so I don't see the benefit to replace the existing and working Winforms version.

@Jklawreszuk
Copy link
Collaborator Author

@Kryptos-FR Alright, I bringed back Winforms report then 👌and I also added the links to the privacy policy again

Copy link
Member

@Kryptos-FR Kryptos-FR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few remarks and an incorrect project reference in the launcher. Otherwise LGTM.

@Kryptos-FR
Copy link
Member

Kryptos-FR commented Sep 11, 2024

@Jklawreszuk I tried to push to your branch but since I'm not a maintainer on this repo I couldn't.

There is a small fix to do for the Thread that is created for the crash report.

  // Stride.Launcher/CrashReport/CrashReportHelper.cs
  var crashLogThread = new Thread(CrashReport) { CurrentUICulture = englishCulture, CurrentCulture = englishCulture };
+ crashLogThread.SetApartmentState(ApartmentState.STA);

Apparently, the [STAThread] attribute on the CrashReport() method is ignored. It might only work on Main().

@Jklawreszuk
Copy link
Collaborator Author

Thanks for the help! I added you to my fork in that case 😅 . Might be useful in future PR's.
Are you able to push your changes now?

@Kryptos-FR Kryptos-FR self-requested a review September 12, 2024 07:58
@tebjan
Copy link
Member

tebjan commented Sep 12, 2024

Will this page still get data and work as before?
https://metrics.stride3d.net/

@Kryptos-FR
Copy link
Member

Will this page still get data and work as before? https://metrics.stride3d.net/

Probably not. I believe the install count is based of nuget so that would stay but the others (like user per month) would be gone.
Maybe @xen2 can confirm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants