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

Alerting: Fix image rendering and uploading timeout preventing to send alert notifications #21536

Merged
merged 11 commits into from Jan 17, 2020

Conversation

marefr
Copy link
Member

@marefr marefr commented Jan 16, 2020

What this PR does / why we need it:
Continuation of #17688 and will make #18011 obsolete/not needed.

Which issue(s) this PR fixes:
Fixes #21018

Special notes for your reviewer:

@marefr marefr marked this pull request as ready for review January 16, 2020 12:50
@marefr marefr mentioned this pull request Jan 17, 2020
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

I think error messages in tests are misleading, unless I'm missing something.

pkg/services/alerting/notifier_test.go Outdated Show resolved Hide resolved
pkg/services/alerting/notifier_test.go Outdated Show resolved Hide resolved
pkg/services/alerting/notifier_test.go Outdated Show resolved Hide resolved
pkg/services/alerting/notifier_test.go Outdated Show resolved Hide resolved
pkg/services/rendering/plugin_mode.go Show resolved Hide resolved
@marefr marefr requested a review from aknuds1 January 17, 2020 10:48
Copy link
Contributor

@aknuds1 aknuds1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@marefr marefr merged commit 71ffd1d into master Jan 17, 2020
@marefr marefr deleted the 21018_fix branch January 17, 2020 11:07
@marefr
Copy link
Member Author

marefr commented Jan 17, 2020

@hekmon thanks for your initial work and contributing to Grafana

johntdyer pushed a commit to johntdyer/grafana that referenced this pull request Jan 21, 2020
…d alert notifications (grafana#21536)

* svc alerting - use a shorter ctx to upload the img
This will prevent timeout on img upload to cancel the notifications from being sent

* components img uploader - pass the ctx to aws lib

* make webdavuploader use the ctx

* make azureblobuploader use the ctx

* rename uploadImage() to renderAndUploadImage()
for better clarity about what this method work

* Use timeout + 2s for plugin renderer (same as service and phantomjs)

* Make sure that original EvalContext is updated after render and upload

* Verify notification sent even if render or image upload times out

* fix lint

* fixes after review

Co-authored-by: Edouard Hur <3418467+hekmon@users.noreply.github.com>

Fixes grafana#21018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alert is not sent if render hangs
3 participants