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

Fix sla report inconsistencies #560

Closed
wants to merge 2 commits into from
Closed

Fix sla report inconsistencies #560

wants to merge 2 commits into from

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Jan 25, 2023

This PR only fixes that the host/service reports with empty history event entries are returning null rather than 100. However, the total report results for the same timeframe are still different depending on whether you select host reports for a week at a time or split them into a 7-day period (in reporting module is called this a breakdown), i.e. instead of for a week at a time, they are only generated for one day at a time and that 7 times.

fixes #558
refs Icinga/icingadb-web#713

@cla-bot cla-bot bot added the cla/signed label Jan 25, 2023
@yhabteab yhabteab force-pushed the fix-sla-reports branch 7 times, most recently from e3f9693 to b997831 Compare January 26, 2023 09:16
@yhabteab yhabteab force-pushed the fix-sla-reports branch 4 times, most recently from 9ca60c8 to c10871d Compare January 26, 2023 20:35
@yhabteab yhabteab marked this pull request as ready for review January 27, 2023 08:14
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Can you please share the data from the SLA history tables you were using (so probably one of the hosts you had for testing in #558).

Comment on lines +73 to +74
AND s.hard_state IS NOT NULL
AND s.previous_hard_state IS NOT NULL
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't these columns already have a NOT NULL constraint?

Comment on lines +163 to +165
-- row count "1" because of the faked ending result used for the
-- cursor loop, whose result set is never used.
IF rowCounts > 1 THEN
Copy link
Contributor

Choose a reason for hiding this comment

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

This would also count downtime rows returned by that query which I doubt you want. But also if this would just count the state rows, this makes the assumption that if there's no event in the interval, there's no valid result to report. Problem with this is that if nothing happens on a checkable, there is no history. So for something that's 100% up, had no downtimes, this would now no longer return a value.

@yhabteab
Copy link
Member Author

Superseded by Icinga/icingadb-web#710

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

Successfully merging this pull request may close these issues.

SLA: Ignore objects that have not yet been checked in a given time period
2 participants