Feature/pagerduty custom details update

[mark-trellix]

If the value of a key:value pair given is not found as a key in an elastalert match then default to using the original value of the key:value pair.

Updated alert document reflecting change and describe logic better.

Description

Expand custom details to allow defaulting to value of provided key:value pair if the value is not found as a key in an elastalert match.

Checklist

• I have reviewed the contributing guidelines.
• I have included unit tests for my changes or additions.
• I have successfully run make test-docker with my changes.
• I have manually tested all relevant modes of the change in this PR.
• I have updated the documentation.
• I have updated the changelog.

Questions or Comments

I noticed when looking at this functionality that this and the logic before these changes assumes that all elastalert matches will each have the value as a key from the provided key:value pair.

See here where it loops over all matches. Following the logic it will only end up assigning/overwriting to the last looped match in matches.

elastalert2/elastalert/alerters/pagerduty.py

Lines 50 to 53 in 55e2659

	if self.pagerduty_v2_payload_custom_details:
	for match in matches:
	for custom_details_key, es_key in list(self.pagerduty_v2_payload_custom_details.items()):
	custom_details_payload[custom_details_key] = lookup_es_key(match, es_key)

If it's assumed all matches are the same, I don't see the need for looping over the matches. Please correct me if I am wrong.

[jertel]

It does seem wasteful, in the scenario where all the records have all fields present. I suspect this was done to accommodate the alternative scenario where some records might have the fields yet others won't. Not a good way to run a datastore but perhaps the person setting up the alerting doesn't have a say into the data that comes in. If you were to remove that behavior then it would have to be considered a breaking change for anyone that has been counting on that behavior. To handle both scenarios a toggle setting would be needed to choose whether to loop or not, and it would have to default to looping.

[mark-trellix]

It does seem wasteful, in the scenario where all the records have all fields present. I suspect this was done to accommodate the alternative scenario where some records might have the fields yet others won't. Not a good way to run a datastore but perhaps the person setting up the alerting doesn't have a say into the data that comes in. If you were to remove that behavior then it would have to be considered a breaking change for anyone that has been counting on that behavior. To handle both scenarios a toggle setting would be needed to choose whether to loop or not, and it would have to default to looping.

That makes sense. Are we good with the changes made here then? They can't expect null value to show up anymore in this case with the changes, as it would now be the value of the key:pair provided.

[jertel]

Yes, it looks good to me. Thanks for submitting it. I'll leave it open for a couple days for others to provide feedback.