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

Jump to bottom

prometheus-xmpp-alerts: 0.4.2 -> 0.5.1; apply RFC 42 #100274

Merged
merged 2 commits into from May 18, 2021
Merged

prometheus-xmpp-alerts: 0.4.2 -> 0.5.1; apply RFC 42 #100274

merged 2 commits into from May 18, 2021

Conversation

hax404
Copy link
Contributor

@hax404 hax404 commented Oct 11, 2020

Motivation for this change

Fix 500s that come back when the alertmanager sends alerts

prometheus-xmpp-alerts[30888]: ERROR    Error handling request
prometheus-xmpp-alerts[30888]: Traceback (most recent call last):
prometheus-xmpp-alerts[30888]:   File "/nix/store/nlvjjvb9471dbpdj8fhdih0ak90cq9d9-python3.8-aiohttp-3.6.2/lib/python3.8/site-packages/aiohttp/web_protocol.py", line 418, in start
prometheus-xmpp-alerts[30888]:     resp = await task
prometheus-xmpp-alerts[30888]:   File "/nix/store/nlvjjvb9471dbpdj8fhdih0ak90cq9d9-python3.8-aiohttp-3.6.2/lib/python3.8/site-packages/aiohttp/web_app.py", line 458, in _handle
prometheus-xmpp-alerts[30888]:     resp = await handler(request)
prometheus-xmpp-alerts[30888]:   File "/nix/store/n2axls35979gx2aqdfnl0yafnrffl100-prometheus-xmpp-alerts-0.4.2/bin/.prometheus-xmpp-alerts-wrapped", line 141, in serve_alert
prometheus-xmpp-alerts[30888]:     text = '\n--\n'.join(create_message_full(alert))
prometheus-xmpp-alerts[30888]:   File "/nix/store/n2axls35979gx2aqdfnl0yafnrffl100-prometheus-xmpp-alerts-0.4.2/lib/python3.8/site-packages/prometheus_xmpp/__init__.py", line 57, in create_message_full
prometheus-xmpp-alerts[30888]:     alert['annotations']['summary'],
prometheus-xmpp-alerts[30888]: KeyError: 'summary'
prometheus-xmpp-alerts[30888]: INFO     ::1 [11/Oct/2020:20:18:38 +0000] "POST /alert HTTP/1.1" 500 244 "-" "Alertmanager/0.21.0"
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from fpletz October 11, 2020 20:47
@maralorn
Copy link
Member

Result of nixpkgs-review pr 100274 1

1 package built:
  • prometheus-xmpp-alerts

@maralorn
Copy link
Member

The fix is apparently in this commit: jelmer/prometheus-xmpp-alerts@8b4c2f7 but I see no harm in just bumping to the newest commit. It includes a commit by @andir ;-) I saw.

This looks fine by me. I am just not sure if there is some other convention for the version name that we should use.

I would like a second opinion on that last point, but ping me, if you don‘t get any more reactions.

mweinelt
mweinelt requested changes Oct 12, 2020
View reviewed changes
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

Please report this issue upstream and have them issue a fixed release.

@maralorn
Copy link
Member

@mweinelt The issue is already fixed upstream. (As I pointed to in the commit I linked.)

Do you want to wait with this PR until they have made the next release?

@mweinelt
Copy link
Member

I'm saying let's apply the patch and wait for upstream to do a new release. Let's not start doing releases downstream.

@maralorn
Copy link
Member

Fair enough.

@andir
Copy link
Member

andir commented Oct 12, 2020

You want at least 0f54da76ecd200887e7f267113b7c6a9cfae43d4 & 500a316a83b5d72c490cb62c3e7f5aa5efc7f5bf from my fork in order to actually be able to use the amtool integration of this. I filed a PR for those just now: jelmer/prometheus-xmpp-alerts#19

@andir
Copy link
Member

andir commented Oct 24, 2020

While we are waiting for upstream and/or the required fixes you can probably start adding a nixos module based on https://github.com/andir/infra/blob/e078bda5b9dafbfff2216a8e9131ce180196c362/config/servers/mon/xmpp-alerts.nix.

@andir
Copy link
Member

andir commented Oct 29, 2020

Please update to the latest release: jelmer/prometheus-xmpp-alerts@1620e9e

@SuperSandro2000
Copy link
Member

@hax404 ping

@hax404 hax404 changed the title prometheus-xmpp-alerts: 0.4.2 -> 0.4.2 + fixes prometheus-xmpp-alerts: 0.4.2 -> 0.5.1 Apr 29, 2021
@hax404 hax404 changed the title prometheus-xmpp-alerts: 0.4.2 -> 0.5.1 prometheus-xmpp-alerts: 0.4.2 -> 0.5.1; apply RFC 42 Apr 29, 2021
@maralorn
Copy link
Member

LGTM, but other people in this thread are more familiar with these parts of nixpkgs.

mweinelt
mweinelt reviewed May 17, 2021
View reviewed changes
Copy link
Member

@mweinelt mweinelt left a comment

Choose a reason for hiding this comment

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

The package looks fine, the module can be migrated further to RFC 42 still, also in a backward compatible way.

nixos/modules/services/monitoring/prometheus/xmpp-alerts.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/prometheus/xmpp-alerts.nix Outdated Show resolved Hide resolved
nixos/modules/services/monitoring/prometheus/xmpp-alerts.nix Outdated Show resolved Hide resolved
@mweinelt mweinelt merged commit 4c79885 into NixOS:master May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mweinelt mweinelt mweinelt requested changes

@andir andir andir requested changes

@fpletz fpletz Awaiting requested review from fpletz

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@hax404 @maralorn @mweinelt @andir @SuperSandro2000