monitoring: configure alert notifications from site config#11427
Conversation
…ribution/alert-notifications
…ribution/alert-notifications
…ribution/alert-notifications
…ribution/alert-notifications
…ribution/alert-notifications
…ribution/alert-notifications
|
wow, super nice. i'll look at it in a bit |
uwedeportivo
left a comment
There was a problem hiding this comment.
not sure what you can do about the grafana credentials
| // update board | ||
| _, err = c.grafana.SetDashboard(ctx, *homeBoard, sdk.SetDashboardParams{Overwrite: true}) | ||
| if err != nil { | ||
| problems = append(problems, newObservabilityAlertsProblem(fmt.Errorf("failed to update dashboard: %w", err))) |
There was a problem hiding this comment.
You have a logic error the linter caught here and in the return cases above. After appending to problems, c.problems isn't updated so this line does nothing.
| http_port = 3370 | ||
| # The (internal) http port to use | ||
| # In `sourcegraph/grafana`, 3370 is reverse-proxied to 3371 | ||
| http_port = 3371 |
There was a problem hiding this comment.
Oops, I just realized that in the single-container Docker image sourcegraph/server we bundle just the Grafana server binary and use a different config file than this one. This may be a blocker to merging this PR, unless we state in the CHANGELOG / docs that observability.alerts does not work in single-container deployments (which would be a little odd)
There was a problem hiding this comment.
The release branch will be created Mon, so if you want to land this change in the release (I think we should) will either need to address this before-hand or document that it doesn't work in sourcegraph/server yet and link to a github issue (that sounds fine to me & better than not having this in the release).
There was a problem hiding this comment.
I've created https://github.com/sourcegraph/sourcegraph/issues/11473 as follow-up and will include it in the changelog!
emidoots
left a comment
There was a problem hiding this comment.
I left more feedback inline, but this is great! Feel free to address my comments as you see fit and then merge (no need to wait for me to review again).
Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
…ribution/alert-notifications
|
@slimsag thank you! sorry to do this so late but I went ahead and wrapped it up for branch cut (completely forgot about that!) |
|
Great! Awesome work! (And get offline it’s the weekend :P) |
* monitoring: configure alert notifications from site config (#11427) * never contribute warning if no notifiers are set * check for grafana-wrapper status code * return specific problem for observability alerts on sourcegraph/server * rename GRAFANA_ to GRAFANA_INTERNAL_, add more docs * indicate 3370 is a useful port with EXPOSE * fix notifier schema * improve changelog wording Co-authored-by: Stephen Gutekanst <stephen.gutekanst@gmail.com>
closes #10641
This PR introduces a new workflow:
site-admin/configurationand adds:Overviewdashboard (duplicate of the home) is updated to fire alerts to the above channels whenx alerts by servicedashboard is >0This is done by a new
grafana-wrapperthat subscribes to site configuration updates and makes API calls to grafana to manipulate the alerts overview dashboard and notification channels.TODO