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

Refactor alerts #179

Merged
merged 15 commits into from Oct 26, 2022
Merged

Refactor alerts #179

merged 15 commits into from Oct 26, 2022

Conversation

trueleo
Copy link
Contributor

@trueleo trueleo commented Oct 22, 2022

Description

Alerts are stored on metadata map which is behind and rwlock. This is fine but having to hold writer lock for alert check is not ideal. This PR refactors code around alerts implementation. Each alerts rule should be self contained with its own interior mutable state if it requires any. Interior mutablity pattern allows for alert to be resolved with shared reference. An alert can now contain different variant of rules which can be state or stateless.


This PR has:

  • been tested to ensure log ingestion and log query works.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

@nitisht
Copy link
Member

nitisht commented Oct 23, 2022

Tested the PR, few observations:

  1. Incorrect alert field causes server to panic. Alert config
{
	"alerts": [{
		"name": "server-fail-alert1",
		"message": "server reported error status",
		"rule": {
			"field": "http_status",
            "operator": "equalTo",
			"value": 500,
			"repeats": 2,
			"within": "5m"
		},
		"targets": [{
			"name": "slack-target",
			"server_url": "https://webhook.site/70659a77-386b-4543-ab1d-98addddceeb1",
			"api_key": ""
		}]
	}]
}

Error:

thread 'actix-rt|system:0|arbiter:4' panicked at 'field exists', server/src/alerts.rs:87:51
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Copy link
Member

@nitisht nitisht left a comment

Choose a reason for hiding this comment

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

The target struct should have format like this

		"target_1": {
			"type": "webhook",
			"url": "https://webhook.site/70659a77-386b-4543-ab1d-98addddceeb1",
			"username": "", //optional
			"password": "" //optional
                         "token": "" //optional
		},

i.e. add a type field, that will allow adding more types later

}
}

#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub enum Operator {
pub enum NumericOperator {
EqualTo,
GreaterThan,
LessThan,
Copy link
Member

Choose a reason for hiding this comment

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

Let's add more rules here NotEqualTo, GreaterThanEquals, LessThanEquals.

#[serde(rename_all = "camelCase")]
pub struct Alerts {
pub alerts: Vec<Alert>,
}

#[derive(Default, Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
#[derive(Debug, Serialize, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct Alert {
pub name: String,
Copy link
Member

Choose a reason for hiding this comment

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

As we discussed, let's add a system generated id here apart from alert name. While saving this to S3, let's use the id as the file name.

#[serde(rename_all = "camelCase")]
pub struct Rule {
pub struct NumericRule {
pub field: String,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of field, let's call it column - makes it simpler to understand

@trueleo
Copy link
Contributor Author

trueleo commented Oct 23, 2022

1. Incorrect alert field causes server to panic. Alert config

field should be checked at time of registering alert. I will add that to validation. It does not make sense to have a fallible code in there.

@nitisht
Copy link
Member

nitisht commented Oct 23, 2022

1. Incorrect alert field causes server to panic. Alert config

field should be checked at time of registering alert. I will add that to validation. It does not make sense to have a fallible code in there.

Yes, I am not asking to validate after saving.

Alerts are stored on metadata map which is behind and rwlock.
This is fine but having to hold writer lock for alert check is not ideal.
This PR refactors code around alerts implementation.

Each alerts rule should be self contained with its own interior mutable state
if it requires any. Interior mutablity pattern allows for alert to be resolved
with shared reference. An alert can now contain different variant of rules
which can be state or stateless.
@nitisht nitisht merged commit 50fa20d into parseablehq:main Oct 26, 2022
@trueleo trueleo deleted the alerts branch October 26, 2022 08:11
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.

None yet

2 participants