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: ai rule gen #2158

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

feat: ai rule gen #2158

wants to merge 37 commits into from

Conversation

mirrormystic
Copy link
Contributor

@mirrormystic mirrormystic commented Oct 9, 2024

Copy link

vercel bot commented Oct 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
keep ⬜️ Ignored (Inspect) Visit Preview Oct 14, 2024 2:23pm

@mirrormystic
Copy link
Contributor Author

pusher_client = get_pusher_client()
if pusher_client:
try:
pusher_client.trigger("private-{}".format(tenant_id), "rules-aigen-created", result)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: f"private-{tenant_id}" is how I see things being done in the code.

Thinking out loud: does this trigger a websocket message to every user logged in with the same tenant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thats a great point, i stole pattern from some other parts of the codebase, should we do something about this? @shahargl

@Kiryous
Copy link
Contributor

Kiryous commented Oct 13, 2024

Now if I don't have correlation rules yet I can't generate rules with AI. I think it's the perfect use case for the feature, so let's maybe always show the tabs with "existing" / "suggested" and add button "Suggest correlations with AI" which would get user to the "Suggested" tab. What do you think?

Screenshot 2024-10-13 at 15 06 56

Copy link
Contributor

@Kiryous Kiryous left a comment

Choose a reason for hiding this comment

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

Awesome work, Haim! Left a suggestion to separate data managing and UI, and few minor comments.

keep-ui/app/rules/AIGenRules.tsx Show resolved Hide resolved
keep-ui/app/rules/CorrelationSidebar/DeleteRule.tsx Outdated Show resolved Hide resolved
keep-ui/app/rules/AIGenRules.tsx Outdated Show resolved Hide resolved
keep-ui/app/rules/AIGenRules.tsx Outdated Show resolved Hide resolved
keep-ui/app/rules/AIGenRules.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@Matvey-Kuk Matvey-Kuk left a comment

Choose a reason for hiding this comment

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

Sorry to ask 😅, but may I ask to craft some unit-tests?

from keep.api.core.db import get_last_alerts
from keep.api.core.dependencies import get_pusher_client

# Add this import at the top of the file
Copy link
Contributor

Choose a reason for hiding this comment

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

Some leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

keep/api/routes/rules.py Show resolved Hide resolved



def select_right_num_alerts(existing_rules, alerts, max_tokens):
Copy link
Contributor

Choose a reason for hiding this comment

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

Strict typing will make it look more like the rest of the codebase :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, done

logger.info(f"Error generating rules: {e}")
result = {'error': "Error generating rules"}


Copy link
Contributor

Choose a reason for hiding this comment

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

Why those empty lines? 0_o

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it just creates more clarity i feel


result['results'] = [rule for rule in result['results'] if check_cel_rule(rule['CELRule'])]

except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we have a few except Exception in the codebase, but why exactly do we catch it here? If that's a way to avoid retries burning OpenAI quota, it's possible to limit amount of retries in Arq

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arq?, this wont create a retry, it just catchs all sorts of exceptions cuz there's a lof different things going on in the try, catch, i don't really want to deal with them in a case by case basis

background_tasks: BackgroundTasks,
request: Request,
authenticated_entity: AuthenticatedEntity = Depends(
IdentityManagerFactory.get_auth_verifier(["read:rules", "read:alerts"])
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess also read:incidents? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're not reading incidents right now i think

env = celpy.Environment()
ast = env.compile(rule_str)
env.program(ast)
return True
Copy link
Contributor

Choose a reason for hiding this comment

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

If you re-use

class RulesEngine:
here, you'll reduce the risk of diversification between this code and the code which is actually executed on alerts :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't think this is a good idea, would you reuse opening a file and reading all the bytes? meh

return selected_alerts


def check_cel_rule(rule_str):
Copy link
Contributor

Choose a reason for hiding this comment

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

I like is_statement notation for methods returning bool, but it's a matter of taste I know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return {"task_id": task_id}


def ruleGen(task_id, authenticated_entity):
Copy link
Contributor

Choose a reason for hiding this comment

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

CamelCase, but the rest is snake_case ;)

@Matvey-Kuk
Copy link
Contributor

@mirrormystic I see your comments "done" but I con't see commits, is there a chance that you didn't push smth? 🤔

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.

[➕ Feature]: AI Creating Correlation Rules
4 participants