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

Remove unused analytics #2964

Merged
merged 4 commits into from
Mar 27, 2019
Merged

Conversation

yanokwa
Copy link
Member

@yanokwa yanokwa commented Mar 25, 2019

Closes #2916

What has been done to verify that this works as intended?

Searched codebase for all setCategory and removed the calls to the events that generate the most hits (ExternalData, Itemset) or are top events and are not actionable (AuditLogging).

I did not verify on a device or Google Analytics that these events are not being sent.

Why is this the best possible solution? Were any other approaches considered?

Narrowest change possible

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

There should be no user-facing changes

Do we need any specific form for testing your changes? If so, please attach one.

No

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No

Before submitting this PR, please make sure you have:

  • run ./gradlew checkAll and confirmed all checks still pass OR confirm CircleCI build passes and run ./gradlew connectedDebugAndroidTest locally.
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

@yanokwa yanokwa force-pushed the remove-unused-analytics branch from eeb0b74 to 2d3c40f Compare March 25, 2019 23:03
@yanokwa yanokwa requested a review from lognaturel March 25, 2019 23:05
@yanokwa yanokwa force-pushed the remove-unused-analytics branch 2 times, most recently from 83fd2db to c3fc05b Compare March 26, 2019 00:25
@yanokwa yanokwa force-pushed the remove-unused-analytics branch from c3fc05b to 4285854 Compare March 26, 2019 01:25
@grzesiek2010 grzesiek2010 added this to the v1.21 milestone Mar 26, 2019
@@ -1220,12 +1217,6 @@ public InstanceMetadata getSubmissionMetadata() {
// timing element...
v = e.getChildrenWithName(AUDIT);
if (v.size() == 1) {
Collect.getInstance().getDefaultTracker()
Copy link
Member

Choose a reason for hiding this comment

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

I would keep this since it's an evolving feature. It might also be good to log when location is turned on.

@lognaturel
Copy link
Member

Can you say more about audit logging not being actionable? It feels to me like it's important to know whether it's something to keep investing in. I suppose if that's already decided, the logging doesn't matter.

@lognaturel
Copy link
Member

I checked in with @yanokwa in person and he said "people who use the audit log say it's a game changer so we're going to continue investing in it regardless." 👍

@grzesiek2010
Copy link
Member

grzesiek2010 commented Mar 26, 2019

Could we remove logs about using the splash screen as well? Here I noticed they cause crash sometimes. I think those we have collected during last two months are enough to make a decision whether we can remove the screen or not.

@yanokwa yanokwa force-pushed the remove-unused-analytics branch 2 times, most recently from cf8feac to ffba655 Compare March 27, 2019 02:48
@yanokwa yanokwa force-pushed the remove-unused-analytics branch from ffba655 to 134b705 Compare March 27, 2019 03:35
@mmarciniak90
Copy link
Contributor

Tested with success.
I did not notice regression.

Verified on Android: 4.2, 4.4, 5.1, 6.0, 7.0, 8.1

I focused the most on forms:

  • All widgets form, especially widget with image-map appearance
  • form with audit
  • form with data loaded from external CSV file
  • splash screen

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"

@grzesiek2010 grzesiek2010 merged commit 92ab6b1 into getodk:master Mar 27, 2019
@yanokwa yanokwa deleted the remove-unused-analytics branch March 27, 2019 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants