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

Jump to bottom

Disable analytics on CI #3929

Merged
merged 1 commit into from
Sep 1, 2021
Merged

Disable analytics on CI #3929

merged 1 commit into from
Sep 1, 2021

Conversation

nirinchev
Copy link
Member

What, How & Why?

Most CI services will set the environment variable CI to some value, so it's a good idea to check for it and make sure we don't count CI builds as actual developers.

Additionally, it modifies the Gradle analytics plugin to not check for the value of REALM_DISABLE_ANALYTICS since that is what we do on node/electron and React Native for iOS.

☑️ ToDos

  • 📝 Changelog entry

@nirinchev nirinchev requested review from kneth and kraenhansen August 30, 2021 10:47
@nirinchev nirinchev self-assigned this Aug 30, 2021
kraenhansen
kraenhansen reviewed Aug 30, 2021
View reviewed changes
react-native/android/analytics.gradle
Comment on lines +60 to +62
def disableAnalytics = env['REALM_DISABLE_ANALYTICS'] != null
def isCI = env['CI'] != null
if (!disableAnalytics && !isCI) {
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).

kneth
kneth reviewed Aug 30, 2021
View reviewed changes
react-native/android/analytics.gradle
@@ -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.

react-native/android/analytics.gradle
def disableAnalytics= env['REALM_DISABLE_ANALYTICS']
if (disableAnalytics == null || disableAnalytics != "true") {
def disableAnalytics = env['REALM_DISABLE_ANALYTICS'] != 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.

kraenhansen
kraenhansen approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@kraenhansen kraenhansen left a comment

Choose a reason for hiding this comment

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

It's fine by me to simplify the semantics of REALM_DISABLE_ANALYTICS. Have you checked if we have to documentation that needs updating too?

@nirinchev
Copy link
Member Author

The current docs mention setting an env variable, but are not prescriptive about the value, which would be in line with the changes in this PR:

realm-js/lib/submit-analytics.js

Lines 32 to 34 in 48144ae

// debated adding this for a long long time. If you truly, absolutely
// feel compelled to not send this data back to Realm, then you can set an env
// variable named REALM_DISABLE_ANALYTICS.

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

Lines 40 to 42 in 48144ae

// relevant data to build a better product for you. If you truly, absolutely
// feel compelled to not send this data back to Realm, then you can set an env
// variable named REALM_DISABLE_ANALYTICS. Since Realm is free we believe

analytics.gradle doesn't contain a similar text, but I can add it there for consistency. I don't believe we have docs outside the source code for this.

@kneth
Copy link
Contributor

kneth commented Aug 30, 2021 •
edited
Loading

@nirinchev Once #3928 #3931 is merged, you can rebase and the failing CI jobs should be fixed

@kneth
Copy link
Contributor

kneth commented Sep 1, 2021

Merging as I don't believe the jest test runner is a real issue

@kneth kneth merged commit aadefa3 into master Sep 1, 2021
@kneth kneth deleted the ni/analytics branch September 1, 2021 07:31
@kneth kneth mentioned this pull request Sep 7, 2021
Merged
1 task
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kneth kneth kneth left review comments

@kraenhansen kraenhansen kraenhansen approved these changes

Assignees

@nirinchev nirinchev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nirinchev @kneth @kraenhansen