Alerting: MuteTiming service return errutil + GetTiming by name #79772
Conversation
This file contains 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
yuri-tceretian
added
area/alerting
yuri-tceretian
requested review from
rwwiv,
JacobsonMT and
grobinson-grafana
and removed request for
a team
yuri-tceretian
force-pushed
the
yuri-tceretian/mute-timing-service-refactor
branch
from
7644446
to
af3ceba
Compare
yuri-tceretian
force-pushed
the
yuri-tceretian/mute-timing-service-refactor
branch
from
af3ceba
to
48fec7a
Compare
yuri-tceretian
force-pushed
the
yuri-tceretian/mute-timing-service-refactor
branch
from
48fec7a
to
db7c716
Compare
yuri-tceretian
changed the base branch from
main
to
yuri-tceretian/mute-timings-tests
yuri-tceretian
force-pushed
the
yuri-tceretian/mute-timings-tests
branch
from
40a987d
to
df0f18f
Compare
yuri-tceretian
force-pushed
the
yuri-tceretian/mute-timing-service-refactor
branch
from
c13534f
to
0f7a8bb
Compare
yuri-tceretian
force-pushed
the
yuri-tceretian/mute-timing-service-refactor
branch
from
0f7a8bb
to
457e2f5
Compare
and update spec to mention the code
Mute timings are deprecated in Alertmanager and will be replaced by time intervals in future
yuri-tceretian
force-pushed
the
yuri-tceretian/mute-timing-service-refactor
branch
from
457e2f5
to
61da747
Compare
rwwiv
approved these changes
Jan 9, 2024
| } | ||
|
|
||
| func (srv *ProvisioningSrv) RouteGetMuteTimingExport(c *contextmodel.ReqContext, name string) response.Response { | ||
| timings, err := srv.muteTimings.GetMuteTimings(c.Req.Context(), c.SignedInUser.GetOrgID()) | ||
| if err != nil { | ||
| return ErrResp(http.StatusInternalServerError, err, "") | ||
| return response.ErrOrFallback(http.StatusInternalServerError, "failed to get mute timings", err) |
There was a problem hiding this comment.
mute timings -> time intervals
| @@ -272,15 +268,15 @@ func (srv *ProvisioningSrv) RouteGetMuteTimingExport(c *contextmodel.ReqContext, | |||
| func (srv *ProvisioningSrv) RouteGetMuteTimings(c *contextmodel.ReqContext) response.Response { | |||
| timings, err := srv.muteTimings.GetMuteTimings(c.Req.Context(), c.SignedInUser.GetOrgID()) | |||
| if err != nil { | |||
| return ErrResp(http.StatusInternalServerError, err, "") | |||
| return response.ErrOrFallback(http.StatusInternalServerError, "failed to get mute timings", err) | |||
There was a problem hiding this comment.
mute timings -> time interval
| } | ||
| return response.JSON(http.StatusOK, timings) | ||
| } | ||
|
|
||
| func (srv *ProvisioningSrv) RouteGetMuteTimingsExport(c *contextmodel.ReqContext) response.Response { | ||
| timings, err := srv.muteTimings.GetMuteTimings(c.Req.Context(), c.SignedInUser.GetOrgID()) | ||
| if err != nil { | ||
| return ErrResp(http.StatusInternalServerError, err, "") | ||
| return response.ErrOrFallback(http.StatusInternalServerError, "failed to get mute timings", err) |
There was a problem hiding this comment.
mute timings -> time intervals
| return ErrResp(http.StatusBadRequest, err, "") | ||
| } | ||
| return ErrResp(http.StatusInternalServerError, err, "") | ||
| return response.ErrOrFallback(http.StatusInternalServerError, "failed to create mute timing", err) |
| if name == timing.Name { | ||
| return response.JSON(http.StatusOK, timing) | ||
| } | ||
| return response.ErrOrFallback(http.StatusInternalServerError, "failed to get mute timing by name", err) |
| } | ||
| return response.JSON(http.StatusAccepted, updated) | ||
| } | ||
|
|
||
| func (srv *ProvisioningSrv) RouteDeleteMuteTiming(c *contextmodel.ReqContext, name string) response.Response { | ||
| err := srv.muteTimings.DeleteMuteTiming(c.Req.Context(), name, c.SignedInUser.GetOrgID()) | ||
| if err != nil { | ||
| return ErrResp(http.StatusInternalServerError, err, "") | ||
| return response.ErrOrFallback(http.StatusInternalServerError, "failed to delete mute timing", err) |
| @@ -5461,6 +5461,10 @@ | |||
| "responses": { | |||
| "204": { | |||
| "description": " The mute timing was deleted successfully." | |||
| }, | |||
| @@ -63,6 +63,7 @@ import ( | |||
| // | |||
| // Responses: | |||
| // 204: description: The mute timing was deleted successfully. | |||
| // 409: GenericError description: The mute timing is in use | |||
There was a problem hiding this comment.
This is provisioning API and it still uses mute timing terminology. I think we should keep it as it is.
|
Thanks! The reason I kept "mute-timings" in those places is because that's mute timing provisioning API whereas service will be used in a new time interval API, and eventually, mute timings will be deprecated (hopefully in G11) |
yuri-tceretian
requested review from
papagian,
undef1nd and
suntala
and removed request for
a team
yuri-tceretian
deleted the
yuri-tceretian/mute-timing-service-refactor
branch
s0lesurviv0r
pushed a commit
to s0lesurviv0r/grafana
that referenced
this pull request
Feb 3, 2024
…ana#79772) * add get mute timing by name to MuteTimingService * update get mute timing request handler to use the service method * replace validation, uniqueness and used errors with errutils * update mute timing methods return errutil responses * use the term "time interval" in errors bevause mute timings are deprecated in Alertmanager and will be replaced by time intervals in the future. * update create and update methods to return struct instead of pointer
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What is this feature?
This PR refactors MuteTiming service to return errutil.Error that contains statuses.
Adds a new method to get mute timing by name and removes this logic from the request handler.
Replaces errors with errutil.Error that is resolved to response status. It simplifies the API request handler logic.
Updates method UpdateMuteTiming to return an error if mute timing is not found by name. Currently, it returns nil, and nil is translated to a NotFound response by API. This does not change the file-provisioning logic because it determines operation based on the current config.
Note: All errors mention
time intervalinsteadmute timingbecause the latter is deprecated and will be removed in the future. Also, the new "regular" API will use time interval semantic and will reuse the errors.Why do we need this feature?
This will let us reuse this service in a new non-provisioning API and avoid duplication of logic and necessity of separate unit-tests.
Who is this feature for?
Special notes for your reviewer:
Please check that: