Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 alert_processor proc to be based on the ECS correlation logic #63
Refactor alert_processor proc to be based on the ECS correlation logic #63
Changes from 6 commits
be8dbf9
dcfa8c2
3994fe8
9825453
6011087
3854f45
11179a0
1dce935
334e729
6dc2de3
4e7afd8
9b6b4fd
9d570f7
ce43a2e
29e1f9d
0c84ad5
180d128
1134e4c
2970cbf
9f23813
19e8abe
2b19dd0
209cd20
3c742dc
c1d02b2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
why select
*
if all you use is theCORRELATED_ID
?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.
Had kept the SQL to be same as the original, but makes sense, will just select the
correlation_id
.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.
I think you can return
null
now that you have the piece withCOALESCE
below?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.
remove
else
if the firstif
always returns so that it's more clearly an early bail condition on the rest of the functionThere 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.
Ah right makes sense 👍
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.
did you mean
action
?(caught by GPT-4!)
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.
Sorry yes.
Wow 😮
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.
I'm not sure if I follow when this would throw an exception -- could you make it more clear, or remove the exception and just do
match = exec(...)[0]
which would be either the first row object orundefined
?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.
This was to try and catch any unexpected errors here, in order to match the ECS version in terms of exception handling. Will just get the first row 👍
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.
would it be possible to call
UUID_STRING()
here, instead, in something likeCOLLATE(?, UUID_STRING())
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.
Did you mean
COALESCE
instead? Would we want it to serve as a fail-safe, and not removegenerate_uuid()
fromfind_related_correlation_id
?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.
this naming is a little confusing since in other places they are constant strings. since it's only used once, could you move this into the
for
loop directly instead of naming it?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.
use
row
instead ofx
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.
since this queries the db, let's maybe call it
find_related_correlation_id
or something else that hints the database is going to be selected?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.
no need to repeat the key in JS instead of
{x: x}
you can just do{x}
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.
did you determine whether this return value is something that can be recorded? if not, let's remove this as it's misleading. if so, what value do you expect to be returned here?
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.
It doesn't seem like we can store the results, without an insert statement running in the proc, to record these values.
However, the return value here is something like below right now, we could maybe pair these with
alert_id
for more context.But I think since the individual call statement results will still be available for 24 hours in the query history, it might not hurt to return these?