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.

Sign up for GitHub

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

Disable analytics on CI #3929

Merged
merged 1 commit into from
Sep 1, 2021
Merged

Disable analytics on CI #3929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ x.x.x Release notes (yyyy-MM-dd)
* <Either mention core version or upgrade>
* <Using Realm Core vX.Y.Z>
* <Upgraded Realm Core from vX.Y.Z to vA.B.C>
* Disable analytics if the `CI` environment variable is set to some value.

10.7.0 Release notes (2021-8-30)
=============================================================
Expand Down
4 changes: 2 additions & 2 deletions lib/submit-analytics.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,8 @@ function isAnalyticsDisabled() {
// ignore error
}

// Or if the user has specifically opted-out
return "REALM_DISABLE_ANALYTICS" in process.env;
// If the user has specifically opted-out or if we're running in a CI environment
return "REALM_DISABLE_ANALYTICS" in process.env || "CI" in process.env;
}

function sha256(data) {
Expand Down
5 changes: 3 additions & 2 deletions react-native/android/analytics.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,9 @@ class SendAnalyticsTask extends DefaultTask {
def sendAnalytics() {
try {
def env = System.getenv()
def disableAnalytics= env['REALM_DISABLE_ANALYTICS']
if (disableAnalytics == null || disableAnalytics != "true") {
def disableAnalytics = env['REALM_DISABLE_ANALYTICS'] != null
Copy link
Contributor

@kneth kneth Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
def disableAnalytics = env['REALM_DISABLE_ANALYTICS'] != null
def disableAnalytics = env['REALM_DISABLE_ANALYTICS'] != null || env['REALM_DISABLE_ANALYTICS'] == "true" || env['REALM_DISABLE_ANALYTICS'] == "1"

Copy link
Member Author

Choose a reason for hiding this comment

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

!= null is already stronger than == true. If REALM_DISABLE_ANALYTICS is set to true, then it won't be null.

def isCI = env['CI'] != null
Copy link
Contributor

@kneth kneth Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
def isCI = env['CI'] != null
def isCI = env['CI'] != null || env['CI'] == "true" || env['CI'] == "1"

Copy link
Member Author

Choose a reason for hiding this comment

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

Similarly with REALM_DISABLE_ANALYTICS, I think it makes sense to check for existence rather than a particular value. I don't suppose many developers have a CI environment variable set on their machines.

if (!disableAnalytics && !isCI) {
Comment on lines +60 to +62
Copy link
Member

@kraenhansen kraenhansen Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This would have the unfortunate side-effect of disabling analytics when REALM_DISABLE_ANALYTICS=false which does not match the semantics that the previous implementation.
I don't know how we document this environment variable, but I just wanted to make it obvious.

Is this what you meant to do?

Copy link
Member Author

@nirinchev nirinchev Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It matches the semantics for iOS and Node/Electron so it made sense to align on all platforms:

realm-js/lib/submit-analytics.js

Line 99 in 48144ae

return "REALM_DISABLE_ANALYTICS" in process.env;

realm-js/react-native/ios/RealmReact/RealmAnalytics.mm

Line 230 in 48144ae

if (getenv("REALM_DISABLE_ANALYTICS") || !RLMIsDebuggerAttached()) {

I feel like just setting REALM_DISABLE_ANALYTICS to any value should stop analytics submission. For example, realm-js's own CI workflow sets REALM_DISABLE_ANALYTICS to 1 rather than true, and I assume many users might feel these are interchangeable. Rather than special-case truthy and falsy values, it's more straightforward to just check for existence. If a user wants to enable analytics, they should just not set the environment variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the integration tests, we have https://github.com/realm/realm-js/blob/master/.github/workflows/integration-tests.yml#L4 (REALM_DISABLE_ANALYTICS is not null).

Moreover, we should set REALM_DISABLE_ANATYTICS for all test runner in Jenkinsfile (as we do for MacOS: https://github.com/realm/realm-js/blob/master/Jenkinsfile#L486).

send()
}
} catch(all) {}
Expand Down
2 changes: 1 addition & 1 deletion react-native/ios/RealmReact/RealmAnalytics.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ static bool RLMIsDebuggerAttached() {
}

void RLMSendAnalytics() {
if (getenv("REALM_DISABLE_ANALYTICS") || !RLMIsDebuggerAttached()) {
if (getenv("REALM_DISABLE_ANALYTICS") || getenv("CI") || !RLMIsDebuggerAttached()) {
return;
}

Expand Down