feat: remote graphite retries #1085

AleksandrMatsko · 2024-09-19T05:51:35Z

Add retry logic for graphite remote

The retry logic is configured with fields:

retry_seconds: 1 1 1  // means there will be 4 retries with an interval of 1 second between them
health_check_timeout: 6s // time for request in IsAvailable method
health_check_retry_seconds: 1 1 1 // retries for IsAvailable method

Also added new error: ErrRemoteUnavailable to signal user about graphite unavailability.

kissken · 2024-09-19T07:25:47Z

script.sh

@@ -0,0 +1,5 @@
+while [ true ]; do 


пум пум пум

kissken · 2024-09-19T07:28:50Z

metric_source/remote/request.go

-func (remote *Remote) makeRequest(req *http.Request) ([]byte, error) {
-	var body []byte
-
+func (remote *Remote) makeRequest(req *http.Request) (body []byte, isRemoteAvailable bool, err error) {


я пока не поняла :(
зачем буль возвращать... помечу себе как подозрительное место

Перед тем, как читать дальше прошу обратить внимание, на то, что почти все ошибки, возвращаемые из функции makeRequest, созданы с помощью fmt.Errorf, т.к. в них кладутся некоторые параметры (например, статус коды, url запроса, тело ответа).

Буль isRemoteAvailable прокидывается обратно по вызовам. И нужен он для того, чтобы "вышестоящие" функции понимали, что нам делать, с ошибкой, например:

функция makeRequestWithRetries по isRemoteAvailable, принимает решение: попытаться ли снова?

функция Fetch по isRemoteAvailable понимает какого типа ошибку отдавать: ErrRemoteTriggerResponse или ErrRemoteUnavailable.

функция IsAvailable возвращает IsRemoteAvailable и полученную ошибку.

Короче говоря, вместо того, чтобы пытаться понять по ошибке, доступен ли источник, мы это понимаем по этому булю.

Ещё стоит обратить внимание, что в отличие от prometheus, в этой реализации для graphite-remote ретраятся лишь некоторые запросы, а не все. Т.е. в случае некорректного запроса:

для текущей реализации prometheus мы сделаем его Retries раз;

для этой реализации ретраев graphite-remote мы вернём первую полученную ошибку.

Почему мы не можем определять по возвращаемой ошибке, что делать дальше? Таким образом мы поднимаем наверх и ошибку (в которой уже явно сказано доступен он или нет) и рядом булев параметр

в отличие от prometheus, в этой реализации для graphite-remote ретраятся лишь некоторые запросы, а не все

А какие все запросы ретраятся в реализации prometheus remote? Типа что код ошибки не обрабатывается?

А какие все запросы ретраятся в реализации prometheus remote? Типа что код ошибки не обрабатывается?

Да. Я имел в виду, что если нам прилетает ошибка от prometheus api, то ретраи продолжаются независимо от ошибки.

фрагмент кода

for i := 1; ; i++ { var res metricSource.FetchResult res, err = prometheus.fetch(target, from, until, allowRealTimeAlerting) if err == nil { return res, nil } ... }

Может вместо буля заведём перечислимый тип? Чтобы по сигнатуре функции было более понятно, что это значение значит?

mchrome · 2024-10-04T11:17:38Z

metric_source/remote/request.go

+}
+
+func isRemoteUnavailableStatusCode(statusCode int) bool {
+	_, isUnavailableCode := remoteUnavailableStatusCodes[statusCode]


Может лучше просто неравенством проверять, что это 5хх код?

А там есть ещё 401 в списке

А, увидел. Ну мне кажется все равно лучше это условием делать, чем мапу проверять. Нам ведь все остальные 5хх нужны тоже, правильно понимаю?

А какая у нас стратегия ретраев? Если unavailable — ретраим, если получили условную 400 — нет?

Tetrergeru · 2024-10-04T12:52:59Z

clock/clock.go

+	time.Sleep(duration)
+}
+
+// NowUnix returns now time.Time as a Unix time.


Suggested change

// NowUnix returns now time.Time as a Unix time.

// NowUnix returns current time in a Unix time format.

Tetrergeru · 2024-10-04T12:53:44Z

cmd/config.go

@@ -239,21 +241,45 @@ type GraphiteRemoteConfig struct {
 	User string `yaml:"user"`
 	// Password for basic auth
 	Password string `yaml:"password"`
+	// Retry seconds for remote requests divided by spaces
+	RetrySeconds string `yaml:"retry_seconds"`


Кажется, договорились щзаменить на массив строк, каждая из которых парсится как временной промежуток

Может стоит вытащить ретраи в отдельную структуру? Кажется, мы потом сможем её переиспользовать в прометее и других ремоут источниках

Tetrergeru · 2024-10-04T12:56:09Z

interfaces.go

@@ -229,5 +229,6 @@ type PlotTheme interface {
 // Clock is an interface to work with Time.
 type Clock interface {
 	NowUTC() time.Time
+	Sleep(duration time.Duration)


А зачем в интерфейс вытаскивать эту функцию?

Tetrergeru · 2024-10-04T12:57:53Z

metric_source/remote/remote.go

 	if err != nil {
-		return nil, ErrRemoteTriggerResponse{
+		if isRemoteAvailable {


Ух, это сравнение читается странно, я бы может быть инвертировал условие

Типа если ремоут недоступен, возвращаем специфическую ошибку, инчае, по стандартному маршруту

d.anchikov and others added 7 commits June 17, 2022 16:54

feat: Added retry logic to remote api request

19f04a9

refactor: update with master

3122c96

refactor: local checker config

cb08142

refactor: config structs, remove unused functions

c0890f7

refactor: remove comments, add new lines

8c52507

refactor: remote graphite tests

722a211

refactor: remove comment

38cca14

kissken reviewed Sep 19, 2024

View reviewed changes

script.sh Outdated

@@ -0,0 +1,5 @@

while [ true ]; do

Copy link

Member

kissken Sep 19, 2024

Choose a reason for hiding this comment

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

пум пум пум

kissken reviewed Sep 19, 2024

View reviewed changes

AleksandrMatsko added 7 commits September 19, 2024 15:21

refactor: api config

85bc18c

merge master into feat/remote-graphite-retries

48dc77a

refactor: recognising isUnavailable code, refactor tests

f5c96c1

refactor: notifier config

6842542

refactor: remove empty line

497eb29

merge master into feat/remote-graphite-retries

8c00a84

merge master into feat/remote-graphite-retries

6db0ad2

AleksandrMatsko marked this pull request as ready for review October 4, 2024 10:44

AleksandrMatsko requested a review from a team as a code owner October 4, 2024 10:44

AleksandrMatsko requested a review from kissken October 4, 2024 10:52

mchrome reviewed Oct 4, 2024

View reviewed changes

Tetrergeru reviewed Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remote graphite retries #1085

feat: remote graphite retries #1085

AleksandrMatsko commented Sep 19, 2024 •

edited

Loading

kissken Sep 19, 2024

kissken Sep 19, 2024

AleksandrMatsko Oct 1, 2024 •

edited

Loading

kissken Oct 4, 2024 •

edited

Loading

Tetrergeru Oct 4, 2024 •

edited

Loading

AleksandrMatsko Oct 4, 2024

Tetrergeru Oct 4, 2024

mchrome Oct 4, 2024

AleksandrMatsko Oct 4, 2024 •

edited

Loading

mchrome Oct 4, 2024

Tetrergeru Oct 4, 2024

Tetrergeru Oct 4, 2024

Tetrergeru Oct 4, 2024

Tetrergeru Oct 4, 2024

Tetrergeru Oct 4, 2024

Tetrergeru Oct 4, 2024

	// NowUnix returns now time.Time as a Unix time.
	// NowUnix returns current time in a Unix time format.

feat: remote graphite retries #1085

Are you sure you want to change the base?

feat: remote graphite retries #1085

Conversation

AleksandrMatsko commented Sep 19, 2024 • edited Loading

Add retry logic for graphite remote

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AleksandrMatsko Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

kissken Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Tetrergeru Oct 4, 2024 • edited Loading

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

AleksandrMatsko Oct 4, 2024 • edited Loading

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

AleksandrMatsko commented Sep 19, 2024 •

edited

Loading

AleksandrMatsko Oct 1, 2024 •

edited

Loading

kissken Oct 4, 2024 •

edited

Loading

Tetrergeru Oct 4, 2024 •

edited

Loading

AleksandrMatsko Oct 4, 2024 •

edited

Loading