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

feat: [DHIS2-12544] Add verbose logging to rules engine #3480

Merged
merged 11 commits into from
Dec 18, 2023
22 changes: 22 additions & 0 deletions packages/rules-engine/src/RulesEngine.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import type {
IConvertInputRulesValue,
IConvertOutputRulesEffectsValue,
IDateUtils,
Flag,
} from './rulesEngine.types';
import { getRulesEffectsProcessor } from './processors/rulesEffectsProcessor/rulesEffectsProcessor';
import { effectActions, typeof environmentTypes } from './constants';
Expand All @@ -21,18 +22,21 @@ export class RulesEngine {
variableService: VariableService;
dateUtils: IDateUtils;
userRoles: Array<string>;
flags: Flag;

constructor(
inputConverter: IConvertInputRulesValue,
outputConverter: IConvertOutputRulesEffectsValue,
dateUtils: IDateUtils,
environment: $Values<environmentTypes>,
flags?: Flag,
) {
this.inputConverter = inputConverter;
this.outputConverter = outputConverter;
this.valueProcessor = new ValueProcessor(inputConverter);
this.variableService = new VariableService(this.valueProcessor.processValue, dateUtils, environment);
this.dateUtils = dateUtils;
this.flags = flags ?? {};
}

/**
Expand Down Expand Up @@ -114,10 +118,15 @@ export class RulesEngine {
expression,
dhisFunctions,
variablesHash,
flags: this.flags,
onError: (error, injectedExpression) => log.warn(
`Expression with id rule:${rule.id} could not be run. ` +
`Original condition was: ${expression} - ` +
`Evaluation ended up as:${injectedExpression} - error message:${error}`),
onVerboseLog: injectedExpression => log.info(
`Expression with id rule:${rule.id} was run. ` +
`Original condition was: ${expression} - ` +
`Evaluation ended up as:${injectedExpression}`),
});
} else {
log.warn(`Rule id:'${rule.id}' and name:'${rule.displayName}' ` +
Expand Down Expand Up @@ -149,10 +158,15 @@ export class RulesEngine {
expression: actionExpression,
dhisFunctions,
variablesHash,
flags: this.flags,
onError: (error, injectedExpression) => log.warn(
`Expression with id rule: action:${id} could not be run. ` +
`Original condition was: ${actionExpression} - ` +
`Evaluation ended up as:${injectedExpression} - error message:${error}`),
onVerboseLog: injectedExpression => log.info(
`Expression with id rule: action:${id} was run. ` +
`Original condition was: ${actionExpression} - ` +
`Evaluation ended up as: ${injectedExpression}`),
});
}

Expand Down Expand Up @@ -196,4 +210,12 @@ export class RulesEngine {
setSelectedUserRoles(userRoles: Array<string>) {
this.userRoles = userRoles;
}

setFlags(flags: Flag) {
this.flags = flags;
}

getFlags(): Flag {
return this.flags;
}
}
2 changes: 1 addition & 1 deletion packages/rules-engine/src/rulesEngine.types.js
Original file line number Diff line number Diff line change
Expand Up @@ -230,5 +230,5 @@ export type IConvertOutputRulesEffectsValue = {|
|};

export type Flag = {
debug: boolean
verbose: boolean,
}
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,9 @@ export const executeExpression = ({
expression,
dhisFunctions,
variablesHash,
flags = {},
onError,
onVerboseLog,
}: ExecuteExpressionInput) => {
const expressionWithInjectedVariableValues = injectVariableValues(expression, variablesHash);

Expand All @@ -191,6 +193,10 @@ export const executeExpression = ({
removeNewLinesFromNonStrings(expressionWithInjectedVariableValues, expressionModuloStrings),
onError,
);

if (flags.verbose) {
onVerboseLog(expressionWithInjectedVariableValues);
}
} catch (error) {
onError(error.message, expressionWithInjectedVariableValues);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// @flow
import type { RuleVariables } from '../../rulesEngine.types';
import type { Flag, RuleVariables } from '../../rulesEngine.types';
import type { D2Functions, D2FunctionConfig } from '../../d2Functions';

export type ExpressionSet = $ReadOnly<{|
Expand All @@ -23,5 +23,7 @@ export type ExecuteExpressionInput = $ReadOnly<{|
expression: string,
dhisFunctions: D2Functions,
variablesHash: RuleVariables,
flags?: Flag,
onError: ErrorHandler,
onVerboseLog: (expressionWithInjectedVariableValues: string) => void,
|}>;
27 changes: 16 additions & 11 deletions src/components/App/App.component.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,24 @@ import * as React from 'react';
import { Provider } from 'react-redux';
import D2UIApp from '@dhis2/d2-ui-app';
import { AppContents } from './AppContents.component';
import { useRuleEngineFlags } from '../../core_modules/capture-core/rules/useRuleEngineFlags';

type Props = {
store: ReduxStore,
};

export const App = ({ store }: Props) => (
<React.Fragment>
<Provider
store={store}
>
<D2UIApp>
<AppContents />
</D2UIApp>
</Provider>
</React.Fragment>
);
export const App = ({ store }: Props) => {
useRuleEngineFlags();
Copy link
Member

Choose a reason for hiding this comment

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

As far as I can tell, this will cause the App component to re-render every time the url changes. Consequently, all children will render unless we have a PureComponent in the chain. I think it would be beneficial to use a PureComponent as the child where we use this hook (For a function component: Wrap it in a Memo).


return (
<React.Fragment>
<Provider
store={store}
>
<D2UIApp>
<AppContents />
</D2UIApp>
</Provider>
</React.Fragment>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,56 @@ describe('Rules engine', () => {
// then
expect(rulesEffects).toEqual([]);
});

test('The rules engine can enable verbose logging', () => {
// When
rulesEngine.setFlags({ verbose: true });

// Then
expect(rulesEngine.getFlags()).toEqual({ verbose: true });
});

test('Rules are calculated when verbose is set', () => {
const programRules = [
{
id: 'GC4gpdoSD4r',
condition: 'true',
description: 'Show error if hemoglobin is dangerously low',
displayName: 'Hemoglobin error',
programId: 'lxAQ7Zs9VYR',
programRuleActions: [
{
id: 'SWfdB5lX0fk',
content: 'Hemoglobin value lower than normal',
displayContent: 'Hemoglobin value lower than normal',
programRuleActionType: 'SHOWERROR',
},
],
},
];

// When
rulesEngine.setFlags({ verbose: true });
const rulesEffects = rulesEngine.getProgramRuleEffects({
programRulesContainer: { programRuleVariables, programRules, constants },
currentEvent,
dataElements: dataElementsInProgram,
selectedOrgUnit: orgUnit,
optionSets,
});

// then
expect(rulesEffects).toEqual([
{
id: 'general',
type: 'SHOWERROR',
error: {
id: 'SWfdB5lX0fk',
message: 'Hemoglobin value lower than normal ',
},
},
]);
});
});

describe('Program Rule Variables corner cases', () => {
Expand Down
7 changes: 6 additions & 1 deletion src/core_modules/capture-core/rules/rulesEngine.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,9 @@ import {
dateUtils,
} from './converters';

export const rulesEngine = new RulesEngine(inputConverter, outputConverter, dateUtils, environmentTypes.WebClient);
export const rulesEngine = new RulesEngine(
inputConverter,
outputConverter,
dateUtils,
environmentTypes.WebClient,
);
23 changes: 23 additions & 0 deletions src/core_modules/capture-core/rules/useRuleEngineFlags.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// @flow
import { useEffect } from 'react';
import { useLocationQuery } from '../utils/routing';
import { rulesEngine } from './rulesEngine';

export const useRuleEngineFlags = () => {
// This hook is used to set the verbose flag on the rules engine
// based on the verbose query param in the URL

const { verbose } = useLocationQuery();

const updateFlags = (flags) => {
rulesEngine.setFlags({ ...rulesEngine.getFlags(), ...flags });
};

useEffect(() => {
if (verbose === 'true') {
updateFlags({ verbose: true });
} else {
updateFlags({ verbose: false });
}
}, [verbose]);
Copy link
Member

Choose a reason for hiding this comment

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

With this approach we can in theory run the rules engine before the verbose flag has been set, because an useEffect in an inner component will run before this one (in practice it will probably work because we use async functions). Did you put this in an useEffect because side-effects should not be part of the rendering, or because some part of react-router-dom is not fully initialised in the render? The preferred solution here might be to use useLayoutEffect (as that one is fired before useLayout)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No special thoughts behind it, just choosing useEffect over useLayoutEffect by default. Switched now 👍

};
Loading