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

Warn when replicating too many docs #5362

Closed
garethbowen opened this issue Feb 8, 2019 · 19 comments
Closed

Warn when replicating too many docs #5362

garethbowen opened this issue Feb 8, 2019 · 19 comments
Assignees
Labels
Priority: 2 - Medium Normal priority Type: Performance Make something faster
Projects

Comments

@garethbowen
Copy link
Member

Users are trying to replicate more docs sometimes on purpose and sometimes due to misconfiguration. This leads to poor user experience and can impact other users by monopolising server resources. We need to do a better job of communicating the maximum number of recommended docs and also warning when users have been configured incorrectly.

  1. @SCdF Has made a start on an API to return the number of docs a user will replicate - take his PR and get it production ready and and merged. Consider caching to reduce load.
  2. When creating a user through medic-conf or the admin app query this API and warn (but don't block) the configurer if the doc count is too high.
  3. When a user replication request is received log a warning in the API log if the doc count is too high. Consider how to not spam the logs, possibly batching the writes together so it's written once per hour.
  4. When a user does an initial replication show a confirm dialog so they are aware that their doc count is unusually high.

The definition of "too high" is up for debate but I think we should warn at 10,000 docs for now. This can be raised when we improve scalability.

@garethbowen garethbowen added Type: Performance Make something faster Priority: 2 - Medium Normal priority labels Feb 8, 2019
@garethbowen garethbowen added this to To do in 3.7.0 via automation Feb 8, 2019
@Hareet
Copy link
Member

Hareet commented Feb 21, 2019

@derickl This looks like the solution for malabawc on CBHIPP

@garethbowen
Copy link
Member Author

@newtewt is working on a proposal to work out what how we scale with different numbers of docs which would be useful for working out when this warning should be shown.

@dianabarsan dianabarsan self-assigned this Jun 3, 2019
@abbyad
Copy link
Contributor

abbyad commented Jun 24, 2019

This is somewhat related to Server Side Purge #5443

@dianabarsan
Copy link
Member

Ready for AT on 5362-offline-user-info

@dianabarsan dianabarsan moved this from In progress to In AT in 3.7.0 Jul 11, 2019
@ngaruko ngaruko self-assigned this Jul 18, 2019
@ngaruko
Copy link
Contributor

ngaruko commented Aug 26, 2019

There are conflicts on the branch....can you please have a look and resolve them before we merge @dianabarsan ?

@dianabarsan
Copy link
Member

@ngaruko conflicts have been resolved.

@ngaruko
Copy link
Contributor

ngaruko commented Sep 3, 2019

LGTM
Offline user replication:
image

Creating user:
image

ngaruko pushed a commit that referenced this issue Sep 3, 2019
Adds /api/v1/users-info endpoint which

returns the number of docs a user will replicate and a warn flag if this number exceeds the recommended limit.
when queried as an offline user, returns the requester doc count.
when queried as an online user, requires facility_id and role params, plus an optional contact_id param (either GET or POST).
is queried as a new step in webapp bootstrapper. When resulting warn flag is truthy, the user has to confirm to actually continue bootstrapping and start replication. Cancelling redirects to login.
is queried as a new step in admin user creation/updating. When resulting warn flag is truthy, a warning message is displayed. The admin would need to click on "Submit" the 2nd time to actually create or update the user document.
Once an hour, API will log warnings of users that have replicated more than 10 000 docs.
Updates webapp bootstrapper to display correct doc count progression, even when interrupted.

#5362
@ngaruko ngaruko closed this as completed Sep 3, 2019
3.7.0 automation moved this from In AT to Done Sep 3, 2019
@abbyad
Copy link
Contributor

abbyad commented Sep 3, 2019

Based on the commit comment it seems as though the warning for creating a user is shown after clicking the Submit button, and then Submit would need to be clicked again to proceed. This doesn't seem to be UX pattern we've used elsewhere in the app... how could we reuse a more familiar pattern to users?

It seems as though having a warning modal would have been the obvious choice -- but the add/edit user is already in a modal and we don't want to layer modals. We could move the add/edit user modal into the main panel, but that is beyond the scope of this issue.

Some other ideas:

  1. Create a new page within the modal. This could work, but seems like a heavy lift and also out of scope for this issue.
  2. Make the text appear with a warning styling. We'd want to differentiate this from the validation errors since you could proceed despite the warning. We could use the "warning" style from our theme.

image

I would go with option 2, and replace the Do you wish to proceed? text since there is no Yes/No option. Here is some suggested text: Warning! This user would replicate 10499 docs, which exceeds the recommended limit. Edit the Role or Place to make changes as needed, then press Submit to proceed.

Also, since the number of docs is affected by server side purge it would be worth adding a note about that to the user. For instance If there are too many docs for a user consider adjusting the doc purge rules.

[edit: clarified wording after getting confirmation that server side purge is considered in the doc count]

@abbyad abbyad reopened this Sep 3, 2019
3.7.0 automation moved this from Done to In progress Sep 3, 2019
@dianabarsan
Copy link
Member

@abbyad

Also, if the number of docs affected by server side purge it would be worth adding a note about that to the user.

The plan is to update server-side-purge PR so the user-docs API would exclude purged docs and reflect the actual number of docs the user would download - but that required for this PR to be merged first.
It's a "todo" at the moment (https://github.com/medic/medic/pull/5878/files#diff-1cfd7b5c0942df518e43b5cc0bf0ded8R138)

To summarize, you are requesting that the message should be:

"Warning! This user would replicate 10499 docs, which exceeds the recommended limit. Edit the Role or Place to make changes as needed, then press Submit to proceed. If there are too many docs for a user consider adjusting the doc purge rules."

in orange warning style?

@abbyad
Copy link
Contributor

abbyad commented Sep 3, 2019

Yes, that is great, thanks.

@dianabarsan
Copy link
Member

dianabarsan commented Sep 6, 2019

The styling changes are available for AT on 5362-styling.
I have 2 more PR-s related to this issue:

Do we want to branch them out to their separate issues so this one would be closed after the styling changes are ATed?

@abbyad
Copy link
Contributor

abbyad commented Sep 6, 2019

I think it is less about whether it should be done with the separate issue, and more about if you think they should be included with the same release (3.7). If you think it should, then let's include them. If you think they are high risk for regressions, or extensive time to AT, then we should consider putting them in a future release.

@garethbowen
Copy link
Member Author

I think it's ok to use the same issue number and have them all ATed at the same time. Those two PRs look like they're ready for AT, just make sure you document any additional things to test, then we can merge all PRs in one fell swoop!

@dianabarsan
Copy link
Member

The styling changes requested are available for AT on 5362-styling

Further, to accomplish point nbr 2 from the issue description (When creating a user through medic-conf query this API and warn (but don't block) the configurer if the doc count is too high):
In light of how roles are used in production, I've changed the endpoint to accept multiple roles. This change is available here: 5362-array-roles.
The medic-conf update is available on this branch 5362-doc-count-warning. This requires the change in 5362-array-roles to work correctly, so please AT them together.

@dianabarsan dianabarsan moved this from In progress to In AT in 3.7.0 Sep 10, 2019
@ngaruko
Copy link
Contributor

ngaruko commented Sep 10, 2019

Styling UI
image
On user creation
image

@abbyad
Copy link
Contributor

abbyad commented Sep 11, 2019

Nice work, looks great!

@ngaruko
Copy link
Contributor

ngaruko commented Sep 11, 2019

One more edge case scenario to clarify @dianabarsan . Is this supposed to cover the case of an existing user being updated and assigned to a place with too many docs via an API call like so:

Content-Type: application/json
{
  "place":  "8606a91a-f454-56e3-a089-0b686af3c6b7"
}

@dianabarsan
Copy link
Member

@ngaruko If you're updating the user via the Admin app, then yes. If you're cURL-ing api/v1/users, then no.
The endpoint that this feature adds is documented here: https://github.com/medic/medic/blob/5362-array-roles/api/README.md#get-apiv1users-info
It's completely separate from /api/v1/users/, and Admin app & medic-conf call it before actually creating or updating the user.

ngaruko pushed a commit to medic/cht-conf that referenced this issue Sep 12, 2019
…227)

Before creating users from csv, calls api/v1/users-info for each user and displays a warning and prompt when API warns about doc counts.

medic/cht-core#5362
@ngaruko
Copy link
Contributor

ngaruko commented Sep 13, 2019

LGTM -
medic-conf create-users also has the warning.
image

ngaruko pushed a commit that referenced this issue Sep 13, 2019
Updates users-info endpoint to accept multiple roles as a JSON stringified string to support how roles are used in production.

#5362
ngaruko pushed a commit that referenced this issue Sep 13, 2019
Updates styles so all modal error have alert-danger style.
Adds optional error severity to enable warning styles.
Updates too-many-docs configuration warning message.

#5362
@ngaruko ngaruko closed this as completed Sep 13, 2019
3.7.0 automation moved this from In AT to Done Sep 13, 2019
kennsippell pushed a commit that referenced this issue Sep 16, 2019
Updates users-info endpoint to accept multiple roles as a JSON stringified string to support how roles are used in production.

#5362
kennsippell pushed a commit that referenced this issue Sep 16, 2019
Updates styles so all modal error have alert-danger style.
Adds optional error severity to enable warning styles.
Updates too-many-docs configuration warning message.

#5362
kennsippell pushed a commit that referenced this issue Sep 19, 2019
Updates users-info endpoint to accept multiple roles as a JSON stringified string to support how roles are used in production.

#5362
kennsippell pushed a commit that referenced this issue Sep 19, 2019
Updates styles so all modal error have alert-danger style.
Adds optional error severity to enable warning styles.
Updates too-many-docs configuration warning message.

#5362
kennsippell pushed a commit that referenced this issue Sep 22, 2019
Updates users-info endpoint to accept multiple roles as a JSON stringified string to support how roles are used in production.

#5362
kennsippell pushed a commit that referenced this issue Sep 22, 2019
Updates styles so all modal error have alert-danger style.
Adds optional error severity to enable warning styles.
Updates too-many-docs configuration warning message.

#5362
kennsippell pushed a commit that referenced this issue Sep 26, 2019
Updates users-info endpoint to accept multiple roles as a JSON stringified string to support how roles are used in production.

#5362
kennsippell pushed a commit that referenced this issue Sep 26, 2019
Updates styles so all modal error have alert-danger style.
Adds optional error severity to enable warning styles.
Updates too-many-docs configuration warning message.

#5362
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 2 - Medium Normal priority Type: Performance Make something faster
Projects
No open projects
3.7.0
  
Done
Development

No branches or pull requests

5 participants