Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Monitor ENF calculation more closely for better progress feedback (EXPOSUREAPP-2743) #1473

Merged
merged 21 commits into from
Oct 26, 2020

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Oct 25, 2020

Why

We want to display better progress feedback, we want to know when the download is happening and when we are waiting for the ENF to check the keys.

The goal is to show two different progress types, this PR does not yet add that, but adds the necessary base classes. For now the isRefreshing state will be displayed on the risk card if either the transaction is on-going, or the ENF is still calculating.

How

This PR adds the CalculationTracker, which is a set of classes that can track the ENF calcluations that happen outside of our app process.
The idea is:

  • Every time we submit submit key files to the ENF, we store an identifier (the token we also pass to the ENF).
  • Register for both ACTION_EXPOSURE_STATE_UPDATED and ACTION_EXPOSURE_NOT_FOUND
  • When we receive a broadcast for either action, the token is included returned to us by the ENF and we mark the respective entry as finished.

To the rest of the app we expose a CalculationTracker.calculations: Flow<Map<String, Calculation>>:

  • We store the last 5 started calculations.
  • If any of those does not have a finishedAt timestamp set by us, it's still calculating.
  • We consider the calculation still in progress as long as there is at least one unfinished calculation.
  • The data should be accessed via ENFClient which offers easier access with
    • isCurrentlyCalculating(): Flow<Boolean>
    • latestFinishedCalculation(): Flow<Calculation?>

To notice any issues within the ENF that don't throw us an exception, there is a timeout checker running within the calculation tracker.
Every 5 minutes it will evaluate the startedAt timestamps of all known calculations. If it exceeds the timeout (currently 60 minutes, @mlenkeit thoughts on that?, might be a bit high), the calculation is considered "timed out", get the result type "TIMEOUT", and given the current time as finishedAt timestamp (which marks it as no longer "in calculation").

The calculation data is persisted everytime there is an update to the data map. The class CalculationTrackerStorage loads and stores the map as serialized JSON file to our private app storage.

Testing

  • Observe the log for CalculationTracker
  • Start risk level calculations and observe the log and the progress state of the risk cards.

@d4rken d4rken added bug Something isn't working enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers labels Oct 25, 2020
@d4rken d4rken requested a review from a team October 25, 2020 21:49
@d4rken
Copy link
Member Author

d4rken commented Oct 25, 2020

In another PR we should consider replacing the timestamp on the risk card, which currently reflects the last time the download was finished, but it makes more sense if it's the timestamp of the newest tracked calculation (which would IMHO be the fix for #948).

import timber.log.Timber
import kotlin.coroutines.CoroutineContext

class HotDataFlow<T : Any>(
Copy link
Member Author

Choose a reason for hiding this comment

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

This PR also introduces a nice helper class dubbed HotDataFlow.

It's a flowable that

  • Does lazy init. The start value is provided via a suspending function that is only executed if someone subscribes
  • Shares all updates between all subscribers as long as there is at least one subscriber.
  • Allows threadsafe updates to it's contents as it only allows its data to be changed by giving it an "update function" that takes the current value and can return a new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Turned out very nicely.

…transient fields.

GSON sets it to false, and does not eval the properties to set them.
The chance for overlapping calculations is rare with batched key submission,
and if gives us a lower chance of actually being affected by timeouts.
@d4rken
Copy link
Member Author

d4rken commented Oct 26, 2020

I've changed the ENF.isCalculating to only check the state of the newest submission. With batched submission there is only a small chance for overlapping calculations and it reduces the chance that we actually run into a timeout and the user sees a prolonged "progress" state. As we currently submit all key files each time, each new calculation would superseed the previous anyways.

@harambasicluka harambasicluka added this to the 1.6.0 milestone Oct 26, 2020
@vaubaehn
Copy link
Contributor

You invited for feedback. First class. Nothing else to say. 👍

@harambasicluka harambasicluka added the prio PRs to review first. label Oct 26, 2020
@mlenkeit
Copy link
Member

@d4rken as discussed, let's set the timeout to 15 minutes initially, should already be quite conservative.

…n with Maximilian.

In worst case scenarios no calculation exceeded 7 minutes.
ralfgehrer
ralfgehrer previously approved these changes Oct 26, 2020
Allows use to later set global settings (pretty print for testers?),
or hook up custom serializers app-wide.
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

71.4% 71.4% Coverage
0.0% 0.0% Duplication

@BMItr
Copy link
Contributor

BMItr commented Oct 26, 2020

passed deep look & test. lgtm

@d4rken d4rken merged commit 383cd8e into release/1.6.x Oct 26, 2020
@d4rken d4rken deleted the feature/2743-better-progress-feedback branch October 26, 2020 19:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working enhancement Improvement of an existing feature maintainers Tag pull requests created by maintainers prio PRs to review first.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants