-
Notifications
You must be signed in to change notification settings - Fork 101
[DC-1071] Field coverage where field is set to default value #465
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
base: master
Are you sure you want to change the base?
[DC-1071] Field coverage where field is set to default value #465
Conversation
Signed-off-by: Nilton Junior <[email protected]>
Signed-off-by: Nilton Junior <[email protected]>
… to specify values to skip Signed-off-by: Nilton Junior <[email protected]>
Signed-off-by: Nilton Junior <[email protected]>
Signed-off-by: Nilton Junior <[email protected]>
lucasfrancoqueiroz2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NiltonGMJunior the code looks good to me. I just have too points:
- I believe the default value of SPIDERMON_FIELD_COVERAGE_SKIP_FALSY should be True and we should set as default the most common empty placeholders for SPIDERMON_FIELD_COVERAGE_SKIP_VALUES. This is a conservative approach that will force the developer to consciously change the settings if needed and avoids the issue to be hidden until being discovered.
- It seems some checks were not successful. Can you check?
Signed-off-by: Nilton Junior <[email protected]>
Hi @lucasfrancoqueiroz2! Sure, we can set the default behaviour to True for the SKIP_FALSY setting. As for the default values to be skipped, we can set a default list, instead of an empty one, but we would need to agree on what those values are. Let me know if you have some suggestions, and I'll gladly make the changes. Thanks for pointing out the checks, I ran black and it seems to have fixed a few minior linting errors now. |
… of skip falsy to True Signed-off-by: Nilton Junior <[email protected]>
Signed-off-by: Nilton Junior <[email protected]>
|
Hi @lucasfrancoqueiroz2! I've updated the PR with the request changes. The doc should also be updated as well as the PR description, with some additional context on this issue. Thanks! |
This PR aims to solve the issue set by DC-1071, from the monitoring chapter.
The issue states that default/falsy values can distort the real field coverage. For example, a value of 0, "N/A", "TBD", "" (empty string), could be considered valid and count towards the field coverage, which is often undesirable.
We already have a config that allows users to skip a field when it's None/null (as oposed to being not present in the item) - SPIDERMON_FIELD_COVERAGE_SKIP_NONE.
This PR proposes two new fields:
SPIDERMON_FIELD_COVERAGE_SKIP_FALSY - skips Python's falsy values, set by the next setting
SPIDERMON_FIELD_COVERAGE_SKIP_VALUES - allows the user to set a list of default values to be skipped when counting values for field coverage.
It's also worth noting that SPIDERMON_FIELD_COVERAGE_SKIP_FALSY defaults to True and the default SPIDERMON_FIELD_COVERAGE_SKIP_VALUES define the falsy value project-wide as: '' (empty string), [], {}, 'N/A' and '-'. This list does NOT include None, which is handled by the existing SPIDERMON_FIELD_COVERAGE_SKIP_NONE setting.