DEV-43744-alert-eval-api #1

yasmin-tr · 2024-02-11T17:07:35Z

What is this feature?

Logzio Alert Evaluation API

contains basic functionality for alert evaluation api
including headers and necessary files and models to support evaluation on logzio datasources

Explaining the main logic and changes:
We added a new internal api for evaluating alerts externally from the alert-manager.
External in this meaning is that the evaluation is called outside the alert-manager and not as part of the engine schedule as it is today.
To be able to reuse the evaluation code as much as possible, we are using the ScheduleService code to run alert evaluation.
In order to use ScheduleService, but without having it running the evaluation, I needed to add a configuration not to execute the evaluations from the scheduler. That way we have all the management of the rule routines and updates, but only the execution is disabled, and then can run from our API.
For running the external evaluation I also added another function for the Scheduler interface that the API is using.

External notification support -
In order to send the notification in an external API, added a LogzioRouter that implements the Send interface which sends the PostableAlerts to the url coming from configuration

Logzio datasources support required the following changes:

adding headers that contains the user-context passed by the API
propagating the headers into the evaluation request in relevant clients - elasticsearch and prometheus
adding url-override from the evaluation request passed by the API
overriding the url of the datasource for the relevant one so it will call the logzio APIs for query

Why do we need this feature?

metrics alerts evaluation process

Who is this feature for?

Logzio Metrics Alerts

Which issue(s) does this PR fix?:

DEV-43744
DEV-43883
DEV-43889

Added code from the following tickets:

DEV-31493 Override datasource URL and pass custom headers to alert rule evaluator (partly)
DEV-17927 - add LogzIoHeaders, Add error msg, change time to StartTime
DEV-30169: api alert evaluation
DEV-16492 Support external alert evaluation
(no ticket) Upgrade to 8.4.0
DEV-18005 - add requestId in error message
DEV-20400 - Grafana alerts evaluation - set 'accountsToSearch' query param

*DEV-31493 Override datasource URL and pass custom headers to alert rule evaluator *DEV-17927 - add LogzIoHeaders, Add error msg, change time to StartTime *DEV-30169, DEV-30170: api alert evaluation and alert processing (processing in comments) *DEV-16492 Support external alert evaluation *(no ticket) Upgrade to 8.4.0 *DEV-18900 - fix For check to be inclusive

Jonathan-Eng

as discussed. the pr is not ready yet for review

Jonathan-Eng · 2024-02-13T10:51:16Z

pkg/api/alerting.go

@@ -9,6 +9,7 @@ import (

 	"github.com/grafana/grafana/pkg/api/dtos"
 	"github.com/grafana/grafana/pkg/api/response"
+	"github.com/grafana/grafana/pkg/models" // LOGZ.IO GRAFANA CHANGE :: DEV-17927 - add LogzIoHeaders


In the comment, I think better to use the reference to the new tickets. Also note in my PR I used a slightly different comment, so better be consistent (you can change mine if you wish).

Jonathan-Eng · 2024-02-13T12:54:31Z

pkg/api/alerting.go

@@ -179,7 +180,8 @@ func (hs *HTTPServer) AlertTest(c *contextmodel.ReqContext) response.Response {
 		return response.Error(400, "The dashboard needs to be saved at least once before you can test an alert rule", nil)
 	}

-	res, err := hs.AlertEngine.AlertTest(c.SignedInUser.GetOrgID(), dto.Dashboard, dto.PanelId, c.SignedInUser)
+	// LOGZ.IO GRAFANA CHANGE :: DEV-17927 - add LogzIoHeaders
+	res, err := hs.AlertEngine.AlertTest(c.SignedInUser.GetOrgID(), dto.Dashboard, dto.PanelId, c.SignedInUser, &models.LogzIoHeaders{RequestHeaders: c.Req.Header})


why are LogzIoHeaders required for test alert flow?

it is technically part of the logzio datasources support, but because a lot of the implementation for the logz evaluation includes this i put it in this PR.

i don't know if at that point it's worth the time of taking out of this PR for the next , because the next PR anyway will be based on that and making these changes can add up a day of work i don't see any value for it 😅
in hindsight i would make the split if i knew about it from the beginning 😓

DEV-18005 - add requestId in error message

DEV-43744 , DEV-43883 , DEV-43895 Removed all code changes related to old alerting which isn't used, therefor not needed.

…-pasting thing relevant code into our API. !!still work in progress!! need to: *remove commented code *add logz headers somehow to request *fix scheduler so it will not evaluate so we can use it for the api *remove processing part

* not returning results in API and keeping the processing part in eval * added a logzio router to replace the regular router and send notifications to logz internal api (in config) * removed unused code of processing * changed the evaluation api to run as bulk

ohadza · 2024-03-24T10:07:45Z

pkg/services/ngalert/api/api.go

@@ -2,6 +2,7 @@ package api

 import (
 	"context"
+	"github.com/grafana/grafana/pkg/services/ngalert/schedule"


mark with // LOGZ.IO GRAFANA CHANGE

ohadza · 2024-03-24T10:08:13Z

pkg/services/ngalert/api/api.go

@@ -75,6 +76,7 @@ type API struct {
 	Tracer               tracing.Tracer
 	AppUrl               *url.URL
 	UpgradeService       migration.UpgradeService
+	Schedule             schedule.ScheduleService


mark with // LOGZ.IO GRAFANA CHANGE

ohadza · 2024-03-24T10:16:14Z

pkg/services/ngalert/api/api_logzio_alerting.go

+}
+
+// NewLogzioAlertingApi creates a new LogzioAlertingApi instance
+func NewLogzioAlertingApi(service *LogzioAlertingService) *LogzioAlertingApi {


Didn't we say its redundant? (leave only the service)

this is the structure of all APIs in this project. there is a class of "XXXApi" that has the route to the endpoint, and the function to call the service class that contains the main logic.
i think what we said is that maybe the LogzioAlertingService can be redundant and we can put the logic directly in this class. but actually i think this structure has some sense in it.. also i'm pretty sure we'll want it also for later when more logic/apis will be added (or at least it gives the infra for it).

ohadza · 2024-03-24T10:22:39Z

pkg/services/ngalert/api/tooling/definitions/alertmanager.go

@@ -4,6 +4,9 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+
+	"github.com/grafana/grafana/pkg/services/ngalert/models"
+	// LOGZ.IO GRAFANA CHANGE :: End


where is the start :)

probably some merge messed this up 😅 will fix it 👍

ohadza · 2024-03-24T12:40:22Z

pkg/services/ngalert/schedule/schedule.go

@@ -467,6 +551,8 @@ func (sch *schedule) ruleRoutine(grafanaCtx context.Context, key ngmodels.AlertR
 				attribute.Int64("results", int64(len(results))),
 			))
 		}
+
+		logger.Debug("RuleRoutine evaluation: processing evaluation results")


you should cleanup eventually all the logger.Debug(...) and comments ....

pkg/promlib/querydata/request.go

pkg/promlib/client/client_test.go

yasmin-tr · 2024-03-28T05:59:59Z

pkg/promlib/querydata/request.go

 	traceCtx, end := s.trace(ctx, q)
 	defer end()

 	logger := s.log.FromContext(traceCtx)
-	logger.Debug("Sending query", "start", q.Start, "end", q.End, "step", q.Step, "query", q.Expr)
+	logger.Debug("fetching query", "query", q, "headers", headers)


will fix this to be the original - for some reason didn't have the log so i added it , not sure how this happened

…lert-eval-api # Conflicts: # pkg/services/ngalert/api/api.go # pkg/services/ngalert/schedule/schedule.go # pkg/setting/setting_unified_alerting.go

pkg/services/authn/clients/proxy.go

* Renamed logzio files to same standard * Changed request object for logzio router notification API and changed default url behaviour

yasmin-tr requested review from Jonathan-Eng and ohadza February 11, 2024 17:08

Jonathan-Eng reviewed Feb 14, 2024

View reviewed changes

yasmin-tr added 6 commits February 14, 2024 15:01

added error_response.logzio.go

9367eef

DEV-18005 - add requestId in error message

Switched to new jira tickets:

45278bb

DEV-43744 , DEV-43883 , DEV-43895 Removed all code changes related to old alerting which isn't used, therefor not needed.

Fix compilation errors

4c1562d

removed unused code

40f495d

ohadza reviewed Mar 24, 2024

View reviewed changes

yasmin-tr commented Mar 28, 2024

View reviewed changes

pkg/promlib/querydata/request.go Outdated Show resolved Hide resolved

yasmin-tr commented Mar 28, 2024

View reviewed changes

pkg/promlib/client/client_test.go Outdated Show resolved Hide resolved

yasmin-tr commented Mar 28, 2024

View reviewed changes

yasmin-tr changed the base branch from logzio to v10.4.x-logzio April 1, 2024 08:29

yasmin-tr changed the base branch from v10.4.x-logzio to logzio April 1, 2024 08:30

Merge remote-tracking branch 'origin/v10.4.x-logzio' into DEV-43744-a…

46080fc

…lert-eval-api # Conflicts: # pkg/services/ngalert/api/api.go # pkg/services/ngalert/schedule/schedule.go # pkg/setting/setting_unified_alerting.go

yasmin-tr force-pushed the DEV-43744-alert-eval-api branch from 69867a8 to 46080fc Compare April 1, 2024 14:57

cycode-security bot reviewed Apr 1, 2024

View reviewed changes

pkg/services/authn/clients/proxy.go Outdated Show resolved Hide resolved

DEV-43889 - Added datasources support for logzio

0d114f4

yasmin-tr changed the base branch from logzio to v10.4.x-logzio April 1, 2024 15:06

yasmin-tr added 3 commits April 2, 2024 14:17

Merge branch 'v10.4.x-logzio' into DEV-43744-alert-eval-api

ef76ff8

* Fixes from merge

6d38146

* Renamed logzio files to same standard * Changed request object for logzio router notification API and changed default url behaviour

* small fixes (comments , removed redundant code)

3cacb1f

yasmin-tr merged commit 7fd4afe into v10.4.x-logzio Apr 3, 2024
10 of 12 checks passed

yasmin-tr deleted the DEV-43744-alert-eval-api branch September 22, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEV-43744-alert-eval-api #1

DEV-43744-alert-eval-api #1

yasmin-tr commented Feb 11, 2024 •

edited

Loading

Jonathan-Eng left a comment

Jonathan-Eng Feb 13, 2024

yasmin-tr Mar 24, 2024

Jonathan-Eng Feb 13, 2024

yasmin-tr Mar 25, 2024

ohadza Mar 24, 2024

ohadza Mar 24, 2024

ohadza Mar 24, 2024

yasmin-tr Mar 25, 2024

ohadza Mar 24, 2024

yasmin-tr Mar 24, 2024

ohadza Mar 24, 2024 •

edited

Loading

yasmin-tr Mar 28, 2024

DEV-43744-alert-eval-api #1

DEV-43744-alert-eval-api #1

Conversation

yasmin-tr commented Feb 11, 2024 • edited Loading

Jonathan-Eng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ohadza Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yasmin-tr commented Feb 11, 2024 •

edited

Loading

ohadza Mar 24, 2024 •

edited

Loading