This repository was archived by the owner on Sep 30, 2024. It is now read-only.
prom-wrapper: introduce src-prometheus, migrate alerts history#17602
Merged
Conversation
ef9a18f to
16b30d1
Compare
16b30d1 to
e74e594
Compare
Contributor
|
Notifying subscribers in CODENOTIFY files for diff 1751289...092b79f.
|
Codecov Report
@@ Coverage Diff @@
## main #17602 +/- ##
==========================================
- Coverage 51.19% 51.15% -0.04%
==========================================
Files 1721 1720 -1
Lines 86071 86105 +34
Branches 7754 7763 +9
==========================================
- Hits 44062 44045 -17
- Misses 38119 38166 +47
- Partials 3890 3894 +4
|
36 tasks
Contributor
|
i'll look at this tomorrow morning |
uwedeportivo
approved these changes
Jan 26, 2021
uwedeportivo
left a comment
Contributor
There was a problem hiding this comment.
LGTM
avoiding the dash for the package name is unfortunate :-), but i cannot come up with a different name
| // Notify admins if critical alerts are firing. | ||
| AlertFuncs = append(AlertFuncs, observabilityActiveAlertsAlert(prometheusutil.PrometheusURL)) | ||
| // Notify admins if critical alerts are firing, if Prometheus is configured. | ||
| promURL, err := srcprometheus.PrometheusURL() |
Contributor
There was a problem hiding this comment.
i found this slightly weird to read. could srcprometheus.PrometheusURL() instead return the empty string when prometheus is not configured ?
Member
Author
There was a problem hiding this comment.
tried to improve the flow here a bit with https://github.com/sourcegraph/sourcegraph/pull/17602/commits/841ac9c70b36c654f4c89cfa6d724a4520d35f79, now usages should just directly create a client and the client creation will return the appropriate ErrPrometheusNotAvailable that can be checked!
uwedeportivo
approved these changes
Jan 27, 2021
Member
Author
|
Did a final sanity check + one last fix (61b5b71):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Was doing a review of the prom-wrapper code and remembered this old code I wrote lying around for the frontend making a direct query to Prometheus, which powers the alerts overview in the bug reports page. There's a few very manual API requests to the prom-wrapper as well. Sadly it seems I wrote some pretty bad code here and figured it might be a good idea to get this cleaned up a bit. Then I got into it and updated all the frontend->prom-wrapper touch points (more than I recalled) into a new package,
src-prometheusThis PR:
alerts.hideObservabilitySiteAlerts)src-prometheuspackage for the prom-wrapper API, and replaces all touch points with the prom-wrapper with usage of this APIsrc-prometheuspackage in the prom-wrapperPost-this PR I would advocate to move
prom-wrapperout ofdocker-imagesand into a top-levelcmd/prom-wrappersince doing this I realized there's a lot more to prom-wrapper than just a reverse-proxy for alertmanager and prometheus