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

Send the prom gen url as alert source #1117

Merged
merged 2 commits into from
Nov 22, 2017
Merged

Conversation

stuartnelson3
Copy link
Contributor

@stuartnelson3 stuartnelson3 commented Nov 22, 2017

From the documentation at
https://v2.developer.pagerduty.com/docs/send-an-event-events-api-v2

payload.source: required string

The unique location of the affected system, preferably a hostname or FQDN.

Sending the generatorURL isn't technically the
affected system, but an event sent to PD can
encompass multiple alerts. The generatorURL seems
like the correct way to help users debug this.

addresses #1116

From the documentation at
https://v2.developer.pagerduty.com/docs/send-an-event-events-api-v2

payload.source: required string

The unique location of the affected system, preferably a hostname or FQDN.

Sending the generatorURL isn't technically the
affected system, but an event sent to PD can
encompass multiple alerts. The generatorURL seems
like the correct way to help users debug this.
@stuartnelson3
Copy link
Contributor Author

looking for opinions: @brancz @grobie @brian-brazil @beorn7

@brian-brazil
Copy link
Contributor

I believe our current behaviour is correct.

The generator URL isn't right here, as a notification can come from multiple Prometheus servers (and technically generator URLs are per-alert). Randomly picking one alert doesn't seem right to me.

I think the sane default is a link to the alertmanager, and allowing templating if a user knows something about their system that lets them select a better option. Which is what we already have.

@stuartnelson3
Copy link
Contributor Author

Source wasn't being templated, so that change is now made in this pr.

@brian-brazil
Copy link
Contributor

👍

Watch the commit message.

As a sane default we link to alertmanager, but
leave templating available to the user if
something suits their system better.
@stuartnelson3
Copy link
Contributor Author

Updated the commit message to make sense, will merge once tests are green

@stuartnelson3 stuartnelson3 merged commit 3464ab4 into master Nov 22, 2017
@stuartnelson3 stuartnelson3 deleted the stn/pd-v2-send-source branch November 22, 2017 19:51
hh pushed a commit to ii/alertmanager that referenced this pull request Oct 19, 2018
* Update Linux cpufreq collector to use new procfs library functions.
* Split thermal throttle collection to a separate function.
* Add new required fixtures and repack ttar file.

Signed-off-by: Ben Kochie <superq@gmail.com>
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.

2 participants