-
Notifications
You must be signed in to change notification settings - Fork 245
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
Feature/Sink PagerDuty #513
Conversation
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.
Very nice work. I've left some comments, but I just wanted to say that the code was a pleasure to read. Extremely readable and clear with just the right amount of comments.
docs/catalog/sinks/PagerDuty.rst
Outdated
- pagerduty_sink: | ||
name: main_pagerduty_sink | ||
api_key: <api key> # e.g. f653634653463678fadas43534506 | ||
url: <url> # e.g. https://events.pagerduty.com/v2/enqueue/ |
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.
Can the URL here ever change? From the PagerDuty docs I would assume its constant in which case you can define it as a constant in the source code. No need to expose it here as a parameter.
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 was not sure because my PagerDuty account recommended me different URLs,
e.g. https://events.pagerduty.com/generic/2010-04-15/create_event.json
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.
@taivo123 I think we can make it a constant. The other URL is for the v1 API and PagerDuty recommends using the v2 API when possible.
} | ||
|
||
headers = {"Content-Type": "application/json"} | ||
response = requests.post(self.url, json=body, headers=headers) |
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 we should log an error here. When testing I put in an incorrect API key and it silently fails without notifying the user.
You can do something like this:
if response.status_code != 200:
logging.error(f"Error sending message to PagerDuty: {response.status_code}, {response.reason}, {response.text}")
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.
Whoops, my bad. You should check response.ok
and not explicitly check for a status_code of 200. PagerDuty actually returns 202 errors, which are also OK.
No description provided.