Skip to content

Conversation

@benjaminysmith
Copy link
Contributor

Description

Update SirCAL logging to support replicating alerts through elastic

Changelog

  • Log aggregated SLA violations
  • Change some messages we don't need in the logs back to prints

Fixes

@benjaminysmith benjaminysmith requested a review from sgsmob February 8, 2021 22:22
Copy link
Contributor

@sgsmob sgsmob left a comment

Choose a reason for hiding this comment

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

Just a few questions about what is going on here but otherwise looks good


if len(complaints) > 0:
for complaint in complaints:
LOGGER.critical(event="signal out of SLA",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be moved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old: each line-level complaint was being logged one at a time (one line per type/source/signal/geo combo)
New: only the aggregated complaint is logged (one line per type/source; signal/geo aggregated)

(this makes it much more useful for alerting)

data_source=complaint.data_source,
signal=complaint.signal,
geo_types=complaint.geo_types,
last_updated=complaint.last_updated.strftime("%Y-%m-%d"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This argument is dropped below. Was that intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in the new code it's been pulled directly into the aggregated message.

@benjaminysmith benjaminysmith requested a review from sgsmob February 9, 2021 15:01
@krivard krivard merged commit c4a5e9f into cmu-delphi:main Feb 9, 2021
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