Opened 7 months ago

Closed 3 months ago

Last modified 2 months ago

#4672 closed change (fixed)

Introduce HockeyApp event-based error reporting

Reported by: mario Assignee:
Priority: P3 Milestone: Adblock-Browser-for-iOS-1.5.2
Module: Adblock-Browser-for-iOS Keywords:
Cc: pavelz, lisabielik, scheer, mario Blocked By: #4766
Blocking: #4516 Platform: Adblock Browser for iOS
Ready: yes Confidential: no
Tester: Rraceanu Verified working: yes
Review URL(s):

Description (last modified by mario)

Background

As of 2016 HockeyApp provides a service called "HockeyApp Events" which can be used to intercept non-crashing errors/exceptions and collect them as error reports. This function is based on HockeyApp's crash report, which has been introduced with #3272.
Introducing this enables us to better understand potential problems and react faster to non-crashing errors.

What to change

HA Events
Implement HA events as described in this documentation.

Opt-In UI

  • Change Crash Reports to Crash and Error Reports in the settings menu
  • Change SEND CRASH REPORT to SEND CRASH AND ERROR REPORTS in the crash reports settings menu
  • Change Ask me after a crash to Ask me after a Crash or Error in the crash reports settings menu
  • Change Only anonymized data containing device information and crash details are sent. to Only anonymized data containing device information and crash or error details are sent. in the crash reports settings menu
  • Add a modal dialog which is shown as soon as an error is intercepted the same way as the modal which is shown when a crash occurs:
    • Title: Adblock Browser Encountered an Error
    • Description: Would you like to send a report to fix the problem?
    • Button 1: Send Report
    • Button 2: Always Send
    • Button 3: Don't Send

Opt-In Logic

  • As soon as an error is intercepted for the first time, show the modal dialog as described above.
    • Tapping "Send Report" will send the report once
    • Tapping "Always Send" will send the report once and change the error reporting settings to "Always"
    • Tapping "Don't Send" will not send any report
  • As soon as an error is intercepted the next time, base action on the error reporting settings:
    • If "Ask me after a crash" is active, show the modal dialog above
    • If "Always" is active, send the error report without showing the modal dialog again
    • If "Don't Send" is active, send no report without showing the modal dialog again

Restrictions
Due to the fact, that non-crashing errors could happen multiple times in a row, apply the following limitations:

  • Limit the error report dialog to be shown only once every 10 minutes
  • Limit events to be sent once every 10 minutes in order not to slow down ABB or clog the user's internet connection if a repeating error is intercepted in a loop
  • Have a backlog of event types with their count
  • Upon any event, add to backlog, increment count as needed
  • If timeout expired, take 5 most common event types
  • If there is less than 5 types, fill up to 5 repeating counts of available types
  • Upon approval (permanent or once), send and reset timeout
  • Clear the backlog, regardless of whether user allowed or denied sending. Otherwise the sending confirmation request would keep popping up for errors happened over the whole app runtime.

Attachments (3)

config_detail.png (168.6 KB) - added by mario 7 months ago.
config.png (189.1 KB) - added by mario 7 months ago.
modal.PNG (59.6 KB) - added by mario 7 months ago.

Download all attachments as: .zip

Change History (35)

Changed 7 months ago by mario

Changed 7 months ago by mario

Changed 7 months ago by mario

comment:1 Changed 7 months ago by mario

  • Description modified (diff)

comment:2 Changed 7 months ago by mario

  • Cc lisabielik added

@Lisa, could you please review the text changes introduced with this issue (Opt-In UI section)? Thanks!

comment:3 Changed 7 months ago by lisabielik

Change:

Only anonymized data containing containing device information and crash details are send.

to:

Only anonymized data containing device information and crash or error details are sent.

comment:4 Changed 7 months ago by mario

  • Description modified (diff)

Oh, that one slipped through. Thanks! Changed the description.

comment:5 Changed 7 months ago by mario

  • Ready set

comment:8 Changed 6 months ago by pavelz

@mario @lisabielik forgotten texting update needed:
former options per crash reports settings menu: Always/Never/Ask me after a Crash

Third option should still say only ...Crash or rather Crash or Error (evt. Crash/Error) per all other requested changes to that dialog?

comment:9 Changed 6 months ago by mario

  • Description modified (diff)
  • Ready unset

Updated the description. Waiting for lisabielik's feedback.

comment:10 Changed 6 months ago by lisabielik

@mario @pavelz

For consistency, I think we should use:

Ask me after a crash or error

comment:11 Changed 6 months ago by pavelz

@lisabielik so drop the existing capitalization?

comment:12 Changed 6 months ago by lisabielik

hi @pavelz,

Sorry...no. It should be:

Ask me after a Crash or Error

Thanks for catching that.

comment:13 Changed 6 months ago by mario

  • Description modified (diff)

Updated description to reflect text changes

comment:14 Changed 6 months ago by pavelz

@mario the requested opt-in logic breaks the logic already established by HockeyApp SDK. We should follow HA SDK logic, unless the error reporting configuration is meant to be separated from crash reporting. Which i understood is not the intent, to simplify things for the user as much as possible.

Tapping "Send Report" will send the report once and change the error reporting settings to "Ask me after a crash or error"

The crash/error modal dialog appears ONLY if the app config is "Ask me", so changing the settings to it is redundant

Tapping "Always Send" will send the report once and change the error reporting settings to "Always"

This is correct and HA SDK does it.

Tapping "Don't Send" will send the report once and change the error reporting settings to "Never"

This not correct. Choosing not to send one specific crash report must not turn off whole error/crash reporting. HA SDK does not do it for a good reason. There would be too many users who turn off the error/crash reporting completely by rejecting sending one specific crash (being on untrusted network at the moment or whatever). The only way to turn the sending off completely is through the Settings.

comment:15 Changed 6 months ago by pavelz

@mario technical:

Limit the events to be sent to a technical reasonable amount in order not to slow down ABB or clog the user|s internet connection if a repeating error is intercepted in a loop

That could be the same rate as proposed for limiting the error modal popping, i.e. a minute. Having an error every minute is a disaster big enough already to not need receiving the same even more often.

On a second thought, even 1 minute may be unnecessarily short. Make it 5 minutes?

comment:16 Changed 6 months ago by mario

  • Description modified (diff)

The crash/error modal dialog appears ONLY if the app config is "Ask me", so changing the settings to it is redundant

Addressed.

This not correct. Choosing not to send one specific crash report must not turn off whole error/crash reporting

Interesting, I thought HA does handle this otherwise.
Addressed.

On a second thought, even 1 minute may be unnecessarily short. Make it 5 minutes?

Agreed and addressed.

Feel free to mark the ticket ready.

comment:17 Changed 6 months ago by mario

  • Description modified (diff)

comment:18 Changed 6 months ago by mario

I've rephrased this limitation slightly:

Limit duplicate events to be sent once every 5 minutes in order not to slow down ABB or clog the user's internet connection if a repeating error is intercepted in a loop

The reason for that is, that we might "lose" error events in case one single event is firing regularly (e.g. every 10 seconds) and thus prevents other events from firing due to the "one event per 5 minutes"-rule. Thus limiting only "duplicate" events might make sense. Is there still any dangers of running into firing too many events? If so I'd suggest to not only limit duplicate events to one event per 5 minutes, but also limit individual events to e.g. 10 every minute or the like.

Last edited 6 months ago by mario (previous) (diff)

comment:19 Changed 6 months ago by pavelz

This is starting to smell of overengineering. I will rephrase what i said above: the app is supposed to produce errors RARELY, which means at most once per hour of sustained usage. Firing more often than 5 minutes is a type of misbehavior which i genuinely hope for not getting through unit tests and QA.

Let's have only one timeout, please. Actually 10 minutes sounds even better than 5 minutes. Folding of duplicate events would somewhat naturally occur anyway, because the only identifier we can influence is a fixed string - fixed for all events of the same type. On the other hand, one such event represent very small packet on the wire - the event string, timestamp, app build identification - a hundred bytes at most. So sending multiple events in one shot shouldn't be a problem.

So i suggest the following logic:

  • have a backlog of event types with their count
  • upon any event, add to backlog, increment count as needed
  • if timeout expired, take 5 most common event types
  • if there is less than 5 types, fill up to 5 repeating counts of available types
  • if user denied sending (permanent or once), keep growing the backlog until app restart (negligible memory consumption)
  • upon approval (permanent or once), send, clear the backlog and reset timeout

comment:20 Changed 6 months ago by mario

  • Description modified (diff)

Sounds good to me. Addressed in the issue's description and added your proposed logic.

comment:21 Changed 6 months ago by pavelz

  • Blocking 4516 added

comment:22 Changed 6 months ago by pavelz

  • Blocked By 4766 added

comment:23 Changed 4 months ago by mario

  • Milestone set to Adblock-Browser-for-iOS-next
  • Ready set

This hasn't landed yet, but it needs to be included in 1.5.2.
Thus I'm already adding the milestone in order to communicate the intended changes before feature freeze.

comment:24 Changed 4 months ago by pavelz

  • Resolution set to fixed
  • Status changed from new to closed

comment:25 Changed 4 months ago by scheer

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Tester changed from Unknown to Scheer
  • When the user has 'Ask me after a crash or error' selected in 'Crash and Error Reports' and then selects 'Produce Error Now' in the 'Devbuild Settings' section, the user is presented with a modal dialog stating that the report was sent successfully and an option with OK. Once selecting OK, the user is then presented with the option to not send, send or don't ask again. Upon selecting 'don't send' the dialog box disappears and then after a few seconds re-.appears. This continues until the user selects 'send report'.
  • The confirmation that the error has been sent also shows when the user has 'Never' under 'Crash and Error Reports' selected. The user must select OK to confirm that the error has already been sent.

comment:26 Changed 4 months ago by pavelz

  • Cc scheer mario added

The reappearance of second dialog (with 3 buttons) is a bug indeed. All events registered up to the point of clicking "Don't send" must get deleted, not attempted to send again.

The first dialog (with one button "OK") says "Error registered" not "sent successfuly". The dialog is meant to confirm that "Produce Error Now" click was registered, not that the event was sent. The reason is, that clicking "Produce" button may not actually produce any visible feedback due to the event throttling. If you click the button multiple times, the events are registered and deferred, not sent immediately. The throttling was a requested feature. The "Error registered" dialog is purely testing/devbuild feature and will not appear in the release build. Now what to do about the confusion:

  • do not display any dialog at all, rely on the tester clicking correctly
  • give the dialog a different message

comment:27 Changed 4 months ago by pavelz

So @mario, on behalf of comment 26, "Restrictions" of issue description should be changed from

  • If user denied sending (permanent or once), keep growing the backlog until app restart (negligible memory consumption)
  • Upon approval (permanent or once), send, clear the backlog and reset timeout

to something along

  • Upon approval (permanent or once), send and reset timeout
  • Clear the backlog, regardless of whether user allowed or denied sending. Otherwise the sending confirmation request would keep popping up for errors happened over the whole app runtime.

comment:28 Changed 4 months ago by mario

  • Description modified (diff)

Agreed. Changed description.

comment:29 Changed 4 months ago by scheer

From my view, simply changing the wording on the pop-up box would be sufficient. Just so it is clearer that this has not been sent. Otherwise, if this is only for development builds, then maybe we could have 'Dev Notification' or something, so I can define between them in the future.

comment:30 Changed 4 months ago by mario

  • Description modified (diff)

Updated description due to an error in the notification's headline: "Adblock Plus" => "Adblock Browser".
The feature is already being localized. We'll need to fix this after receiving the translations.

comment:31 Changed 3 months ago by mario

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:32 Changed 2 months ago by rraceanu

  • Tester changed from Scheer to Rraceanu
  • Verified working set

Verified working

  • Error report is sent automatically with "Always" option with no notification
  • No report is sent when "Never" is checked with no notification
  • User notification is presented when "Ask me after a Crash or Error" where:
    1. [Send report] - sends a report and closes notification
    2. [Don't send] - doesn't send any report and closes notification
    3. [Always Send] - Sends report, switches setting to "Always" and displays a notification informing the user if he wants to revert the change to follow the instructions

Iphone 6
iOS 9.1
ABB 1.5.2-dev (1507)

Note: See TracTickets for help on using tickets.