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

Enhancing RuleFailureTelemetry #1172

Closed
hithacker opened this issue Oct 19, 2021 · 9 comments
Closed

Enhancing RuleFailureTelemetry #1172

hithacker opened this issue Oct 19, 2021 · 9 comments
Assignees

Comments

@hithacker
Copy link
Collaborator

hithacker commented Oct 19, 2021

Motivation

As an implementor, as soon as seeing a rule failure telemetry entry, I should be able to understand where the failure has occurred. I dont want to spend my time in going through the stacktrace to identify the source of the error. This will speed up my ability to fix issues with rules and we can reduce the tickets.

Context

Rule failures for the web data entry app are being logged in a different table called rule_failure_log instead of rule_failure_telemetry. We did that earlier to have additional useful fields required for the new rule system but we implemented it only on web and not on mobile. E.g. in old rule system we had a uuid for rule but in new rule system there is no rule entity with uuid so we need form id and rule type to identify the rule.

Having two tables makes it difficult to provide a user interface. We should refactor the code to put DEA errors also in the original rule_failure_telemetry table and get rid of rule_failure_log table. To start with we will fix the rule_failure_telemetry table to work with new rules as part of this card.

Acceptance criteria:

  1. a. Add the below fields to rule_failure_telemetry table - Extra fields in rule_error_log that could be useful to add to rule_failure_telemetry.
  • source_type (text) - values can be Decision, VisitSchedule, Validation, EnrolmentSummary, SubjectSummary, WorkListUpdation, EnrolmentEligibilityCheck, EncounterEligibilityCheck, Checklist, FormElement, FormElementGroup, OldRule, ReportCard.
    Note: Even if a old decision rule faced the issue, we need not mention the value as 'Decision' here and instead we will mention as 'OldRule' since the data storage structure of old rules are different and hence we do this to be able to track down to the issue location.
  • source_id (uuid) - this is more of a static entity(answers where the issue is?). This is a basically an enhanced version of the current column rule_uuid. Currently form uuid is inserted as rule_uuid for new rules. For old rules, rule_uuid from rule table is inserted. To avoid the confusion this will cause and to support more sources(like ReportCard) we can store the below in this column:
    - uuid of form if source_type is Decision, VisitSchedule, etc.,
    - uuid of form_element if source_type is FormElement
    - uuid of FormElementGroup if source_type is FormElementGroup
    - uuid of rule in the rule table if source_type is OldRule
    - uuid of report_card if source_type is ReportCard
  • entity_type (text) - values can be Subject, ProgramEnrolment, ProgramEncounter, Encounter, Checklist. Will be null for ReportCard source_type.
  • entity_id (uuid) - this is more of a dynamic entity(answers who faced?). value should be uuid. Will be null for ReportCard source_type.
  • app_type (text) - values can be Web, Android. Need not add in the realm table.
  1. b. Rename close_date_time to closed_date_time to make it inline with the conventions followed for the column names. Need not add in the realm table.
  2. The values should sync from mobile app to server
  3. Display the above additional columns in the rule failures page in the AppDesigner.
  4. Also add a link column with a link icon as value for the rows on click of which the user should be taken to the place where the issue is.
    • For form element group rule, or rules on form - clicking on link icon should take the user to the form
    • For encounter eligibility check rule - take to encounter type
    • For report card issue, take to report card.
    • Similarly for others.
  5. Retain rule_uuid column for backwards compatibility but remove the not null constraint on it.
  6. Also fix the expand_more, expand_less icon that is currently not rendering on the webapp.

Out of scope:

  • storing failures when using DEA is out of scope. And hence is, fixing the server-side code for DEA to save errors to rule_failure_telemetry.
  • MessageRule/ScheduleRule since it is executed in the rules-server.
@hithacker hithacker transferred this issue from avniproject/avni-product Oct 19, 2021
@hithacker hithacker self-assigned this Oct 26, 2021
@hithacker hithacker changed the title Rule failure refactoring Rule failure refactoring - Server Oct 26, 2021
@mahalakshme mahalakshme reopened this Nov 6, 2023
@mahalakshme mahalakshme transferred this issue from avniproject/avni-server Nov 6, 2023
@mahalakshme mahalakshme changed the title Rule failure refactoring - Server Enhancing RuleFailureTelemetry Nov 6, 2023
@vinayvenu
Copy link
Member

Related to avniproject/avni-product#1419

Backwards compatibility requirements needs to be added to the card since this is one place where we might face issues.

@mahalakshme
Copy link
Contributor

@vinayvenu have added this point: Retain rule_uuid column for backwards compatibility but remove the not null constraint on it.
for backward compatibility.

@mahalakshme
Copy link
Contributor

add ability to navigate to the rule failed.

@vinayvenu
Copy link
Member

Sure @mahalakshme . One more thing here is that we can potentially move the rule failure log into a different story.

@mahalakshme
Copy link
Contributor

yeah @vinayvenu I have already added it under out of scope

@himeshr himeshr self-assigned this Nov 10, 2023
himeshr added a commit to avniproject/avni-models that referenced this issue Nov 15, 2023
himeshr added a commit to avniproject/avni-webapp that referenced this issue Nov 15, 2023
himeshr added a commit to avniproject/avni-server that referenced this issue Nov 15, 2023
@himeshr
Copy link
Contributor

himeshr commented Nov 16, 2023

Completed tasks:

1.a Add the below fields to rule_failure_telemetry table -

  • source_type (text)
  • source_id (uuid)
  • entity_type (text)
  • entity_id (uuid)
  • app_type (text)
    1.b Rename close_date_time to closed_date_time
  1. The values should sync from mobile app to server
  2. Display the above additional columns in the rule failures page in the AppDesigner.
  3. Retain rule_uuid column for backwards compatibility but remove the not null constraint on it.
  4. Also fix the expand_more, expand_less icon that is currently not rendering on the webapp.

Not picked up task:

  1. Add a link column with a link icon as value for the rows on click of which the user should be taken to the place where the issue is.

Sample webapp screenshot

Screenshot 2023-11-15 at 6 52 11 PM Screenshot 2023-11-15 at 6 28 04 PM

@himeshr
Copy link
Contributor

himeshr commented Nov 16, 2023

As part of the client code changes done, to include Source and Entity details as part of RuleFailureTelemetry, I have in general followed the principle mentioned in the card description, to indicate the Static context info as part of the "Source" and the dynamic specific instance info as part of the "Entity".
But these are errorCase scenarios in which we capture this information, so there is no guarantee that this would be mapped correctly.

Keeping the above information in mind, and also the effort involved in server and webapp code to correctly transform the Source and Entity information to a specific webpage, and then handling the various navigation aspects of that, i have not picked up the task 4. (Add a link column with a link icon as value for the rows on click of which the user should be taken to the place where the issue is.)

@mahalakshme and @vinayvenu pls let me know your thoughts on whether we should work on 3.b as part of this card.?
Or if it should be done as a separate card or not at all?

himeshr added a commit to avniproject/avni-webapp that referenced this issue Nov 16, 2023
himeshr added a commit to avniproject/avni-webapp that referenced this issue Nov 16, 2023
@himeshr
Copy link
Contributor

himeshr commented Nov 16, 2023

After discussion with @mahalakshme and @vinayvenu , we concluded that 3.b could be handled as part of a separate card.
Additionally, the suggestion mentioned in 3.b could be one of the ways to help Implementers navigate to the right error.
Vinay suggested, making the individualUUID column a link to the individual on DEA and showing rest of the information as Readable Names/ titles and not as UUIDs.

This needs further analysis, so moving this card to CodeReviewReady with only these set of changes.

@AchalaBelokar
Copy link

  • 1.a Add the below fields to rule_failure_telemetry table -

source_type (text)
source_id (uuid)
entity_type (text)
entity_id (uuid)
app_type (text)
1.b Rename close_date_time to closed_date_time
showing this field on this table

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

5 participants