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: replace CPU transformation by lasting evaluation #520

Merged
merged 5 commits into from
Nov 16, 2023
Merged

fix: replace CPU transformation by lasting evaluation #520

merged 5 commits into from
Nov 16, 2023

Conversation

Tulux
Copy link
Contributor

@Tulux Tulux commented Nov 15, 2023

The min(over='1h') transformation will gather minimal values on a rolling period of 1h which doesn't make sense since this monitor intends to display CPU usage. Plus this transformation happens after splunk time aggregation which means values will vary depending on the displayed period time, ie: a 24h window will likely show lower values than a 7 days period since time aggregation is done with mean().

I think it was an error from the original commit.

@pdecat
Copy link
Member

pdecat commented Nov 15, 2023

Could you please provide some rationale about this change?

@Tulux
Copy link
Contributor Author

Tulux commented Nov 16, 2023

@pdecat I added note in first comment

@pdecat
Copy link
Member

pdecat commented Nov 16, 2023

The "idea" of this detector is to raise an alert if the CPU usage never goes down for a long period.
We do not want to alert on every spike.
Also, using CPU is a good thing as we are paying for it.

If this does not fit your use case, just customize the variable.

@Tulux Tulux changed the title fix: remove pointless CPU transformation draft: fix: remove pointless CPU transformation Nov 16, 2023
@Tulux
Copy link
Contributor Author

Tulux commented Nov 16, 2023

The "idea" of this detector is to raise an alert if the CPU usage never goes down for a long period. We do not want to alert on every spike. Also, using CPU is a good thing as we are paying for it.

This should be done in the detect() part where you can set a period detection of 1h via when() which results in a same behavior. Transformation functions are calculated after detector time aggregation and they also hide the original metric which is a barrier for operational run.

If this does not fit your use case, just customize the variable.

Yep I did, however the point of this PR is to provide a correct default detector which brings original metric visualization.

Copy link
Member

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

With the addition of a 1h duration, this is now way better.

LGTM!

@pdecat pdecat self-requested a review November 16, 2023 12:38
Copy link
Member

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

Except the changes must be done in the configuration, not the generated files.

Copy link
Member

@pdecat pdecat left a comment

Choose a reason for hiding this comment

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

LGTM!

@haedri haedri merged commit efcee1f into claranet:master Nov 16, 2023
14 checks passed
@pdecat pdecat changed the title draft: fix: remove pointless CPU transformation fix: replace CPU transformation by lasting evaluation Nov 16, 2023
@Tulux Tulux mentioned this pull request Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants