Sprint 04 #6

fev0ks · 2023-01-23T11:41:17Z

No description provided.

mr-fard4y · 2023-01-27T20:26:29Z

cmd/agent/service/sender/bulk_sender.go

+)
+
+type bulkSender struct {
+	msCtx          context.Context


контекст нежелательно хранить в рамках описания структур
придерживаться правила keep Context out of structs

из офф.документации

использовать контекст в рамках передаче параметров, таймаута при вызовах API

для ознакомления
для ознакомления

mr-fard4y · 2023-01-27T20:29:32Z

cmd/agent/service/sender/bulk_sender.go

+	client         *resty.Client
+	metrics        []*model.Metric
+	batchLimit     int
+	mutex          sync.RWMutex


можно использовать возможности эмбеддинга без указания названия параметра (Anonymous Embedded Fields)

type bulkSender struct { ... sync.RWMutex }

при таком описании вызовы можно сократить до

s.Lock() defer s.Unlock()

mr-fard4y · 2023-01-27T20:45:43Z

cmd/agent/configs/config.go

+	return useBuffSendClient
+}
+
+func GetBuffBatchLimit() int {


реализация методов аналогична пакету
можно переиспользовать его

mr-fard4y · 2023-01-27T21:20:02Z

cmd/agent/configs/config.go

+}
+
+func GetBuffSendInterval() time.Duration {
+	buffSendInterval := os.Getenv("BUFF_SEND_INTERVAL")


BUFF_SEND_INTERVAL - неизменняеые переменные (текст, числа) лучше определять как константы

mr-fard4y · 2023-01-27T21:23:02Z

cmd/agent/configs/config.go

@@ -46,3 +49,37 @@ func GetServerAddress() string {
 func GetHashKey() string {
 	return os.Getenv("KEY")
 }
+
+func UseBuffSenderClient() bool {
+	useBuffSendClient, err := strconv.ParseBool(os.Getenv("USE_BUFF_SEND_CLIENT"))


можно не использовать дополнительный флаг, а использовать ранее описанную переменную BUFF_BATCH_LIMIT:
если значение 0 или -1, то обычная отправка
если значение не 0 (в идеале >= 2), то использовать отправку метрик "пачками"

mr-fard4y · 2023-01-28T12:13:50Z

cmd/agent/service/sender/bulk_sender.go

+		metrics:        make([]*model.Metric, 0, batchLimit),
+		batchMetricsCh: make(chan []*model.Metric),
+	}
+	go sender.startSenderListener(msCtx, sendInterval)


пожелание
порядок описания методов упорядочить по их непосредственному вызову
если метод содержит в себе доп.вызовы, то располагать их сразу же за вызывающей функцией

такой подход увеличивает удобстов и читабельность:
реализации вызываемых функций располагаются рядом и можно просматривать их "сверху вниз", не перескакивая по всему файлу

например,

func myFunc1(): func2() func3() func func2() { func4() ... } func func4() { ... func func3() { ...

для данного файла:

NewBulkMetricSender() startSenderListener() uploadMetrics() sendMetricsAsync() sendMetrics() SendMetric() <- можно в начало, сразу же после "конструктора" т.к. метод публичный

mr-fard4y · 2023-01-28T13:42:39Z

cmd/agent/service/sender/bulk_sender.go

+			ticker.Stop()
+			return
+		}
+		if len(metrics) != 0 {


отправка данных только по интервалу, заданному при старте сервиса:
весь ранее собранный список метрик делится чанки, заданного размера и отправляется на сторону сервера

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

в этом и была идея, если чанк заполнен - отправлять
если не заполнен и что то есть, то отправить через интервал, чтоб данные не зависли до след сбора

такое поведение будет неявным, т.к. нельзя с уверенностью сказать, в какой именно период данные будут отправляться: интенсивно в начале периода, интенсивно в конце периода, хаотично

изначально, в конфигурации аганта уже задан параметр reportInterval, определяющий периоды отправки
до этого момента метрики будут копиться в некотором буфере, а далее отправляться
такое поведение эмулирует штатные ситуации, например:

метрики нужно отсылать с определенной периодичностью - 1/5/10м/30 мин, 1/4/12 часов - системы мониторинга вроде Zabbix, Nagios, Cacti, Prometheus, etc конфигурируются по такому принципу;

на сервере есть ограничение на отправку, которое обнуляется по некоторому периоду - но в такой ситуации лучше дополнительно обрабатывать ответ и повторять отправку n-е количество раз или с некоторой задержкой;

в этих кейсах ожидается, что данные будут отпрявляться только с некоторой периодичностью, но никак иначе

отправку данных сразу же при наполнении буфера можно предусмотреть, когда reportInterval равен 0

mr-fard4y · 2023-01-28T15:18:51Z

cmd/agent/service/sender/bulk_sender.go

+func (s *bulkSender) uploadMetrics() []*model.Metric {
+	s.mutex.Lock()
+	metrics := s.metrics
+	s.metrics = make([]*model.Metric, 0, s.batchLimit)


предложение подумать, над текущим решением и архитектурой
использование make каждый раз переаллоцирует занимаемую под структуру память, что затратно

mr-fard4y · 2023-01-28T15:23:52Z

cmd/agent/service/sender/bulk_sender.go

+	if len(s.metrics) >= s.batchLimit {
+		log.Printf("Flush batch of '%v' metrics\n", len(s.metrics))
+		s.batchMetricsCh <- s.metrics
+		s.metrics = make([]*model.Metric, 0, s.batchLimit)


предложение подумать, над текущим решением и архитектурой
использование make каждый раз переаллоцирует занимаемую под структуру память, что затратно

можно попробовать:

оставить только канал, как единую структуру взаимодействия

использовать слайсы

детально про массив и слайсы
внутреннее устройство
так же посмотреть, как работает append/make с массивами/слайсами, и как меняется адресация, len, cap

mr-fard4y · 2023-01-28T15:28:37Z

cmd/agent/service/sender/bulk_sender.go

+func (s *bulkSender) startSenderListener(ctx context.Context, sendInterval time.Duration) {
+	ticker := time.NewTicker(sendInterval)
+	for {
+		var metrics []*model.Metric


метод усложнен

хотелось бы, чтобы вызовы происходили последовательно, т.е.
при кейсе с тикером - вся лоигка отправки данных
при кейсе с контекстом - завершение и освобождение ресурсов

mr-fard4y · 2023-01-29T18:42:20Z

internal/exit_handler.go

@@ -41,6 +41,7 @@ func (eh *ExitHandler) shutdown() {
 	select {
 	case <-successfullyFinished:
 		log.Println("System finished work, graceful shutdown")
+		//time.Sleep(time.Second)


неиспользуемый код лучше сразу же вычищать
стараться не увеличивать энтропию ( :

mr-fard4y · 2023-01-29T18:52:23Z

cmd/agent/service/sender/bulk_sender.go

+	}
+}
+
+// Looks like useless for current project specific :((((


неиспользумый код лучше сразу же вычищать

нуу ок

mr-fard4y · 2023-01-29T18:59:46Z

cmd/agent/service/collector.go

+}
+
+func (cmr *commonMetricCollector) processGopsMetrics() {
+	metrics := make([]*model.Metric, 0)


можно сразу же задать емкость структуры:
минимум - 3 метрики (общая память, свободная память, загрузка 1го процессора), максимум - динамическое значение, в зависимости от количества ядер

metrics := make([]*model.Metric, 0, 3)

mr-fard4y · 2023-01-29T19:14:50Z

cmd/agent/service/collector.go

+	memoryStat, _ := mem.VirtualMemory()
+	metrics = append(metrics, cmr.mf.NewGaugeMetric("TotalMemory", model.GaugeVT(memoryStat.Total)))
+	metrics = append(metrics, cmr.mf.NewGaugeMetric("FreeMemory", model.GaugeVT(memoryStat.Free)))
+	cpuUsed, _ := cpu.Percent(time.Second*10, true)


"магическое число" в параметре вызова

параметр периода опроса можно задать как 0

If an interval of 0 is given it will compare the current cpu times against the last call.

или пробрасывать из конфигурации

Магическим его сложно назвать, согалсно документации оно задано однозначно и не предполагает изменений в дальнейшем через параметры или флаги :)
Имя метрики: "CPUutilization1", тип: gauge - загрузка ядра в процентах за 10 секунд (точное количество - по числу CPU определяемое во время исполнения)

при изменении ТЗ придется изменять все значения в кода, которые используют данное значение

при выносе в константы или в рамках параметра структуры получаем некоторую параметризацию
и минимальный дифф для изменений

добавил константу

mr-fard4y · 2023-01-29T20:38:35Z

cmd/agent/repositories/repository.go

+func (cmr *commonMetricRepository) GetMetrics() []*model.Metric {
+	cmr.Lock()
+	defer cmr.Unlock()
+	metrics := make([]*model.Metric, 0, len(cmr.cache))


начиная с 1.18 версии, появился аналогичный фунционал получения значений - map.Values

values := maps.Values(<map>)

пакет экспериментальный: использование на свое усмотрение
можно ознакомиться с реализациями, как примеры использования дженериков

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

можно не тащить весь пакет, а только скопипастить реализации

и дополнить некоторым комментом, что взято из некоторого пакета: на случай, если реализации добавят в основные батарейки

mr-fard4y · 2023-01-30T11:40:09Z

cmd/agent/service/poller.go

@@ -49,9 +49,9 @@ func (cmp *commonMetricPoller) PollMetrics() chan struct{} {
 			case <-ticker.C:
 				start := time.Now()
 				log.Println("Poll metrics start")
-				metrics := cmp.mr.GetMetricsList()
+				metrics := cmp.mr.GetMetrics()
 				cmp.sendMetrics(metrics)


текущий вызов может содержать ошибки при выполнении
которые стоит обрабатывать и в зависимости от уровня критичности соответствующе обрабатывать или завершать работу сервиса

обновил

mr-fard4y · 2023-01-30T11:41:49Z

cmd/agent/service/sender/json_sender.go

-		err := js.SendMetrics(js.buffer)
-		if err != nil {
-			return fmt.Errorf("cannot send batch of metrics: %v", err)
+func (js *jsonSender) SendMetrics(ctx context.Context, metrics []*model.Metric) {


методы дублируется в text_sender и bulk_sender

стоит подумать над объединением общего кода и выноса в некоторую абстракцию или независимый метод

text и json согласен,
в bulk все же по другому

выделил разные интерфейсы MetricSender - MetricsSender и композицию для общего Sender

mr-fard4y · 2023-01-30T11:42:15Z

cmd/agent/service/sender/bulk_sender.go

+			case metrics := <-s.batchMetricsCh:
+				log.Printf("Sending bulk %d metrics", len(metrics))
+				err := s.sendMetrics(metrics)
+				if err != nil {


хорошая практика возвращать ошибку в рамках выполнения метода
но не "глушить" явно

обновил

mr-fard4y · 2023-01-30T11:46:20Z

cmd/agent/service/sender/bulk_sender.go

+//}
+
+func (s *bulkSender) startSendMetricsListener(ctx context.Context) {
+	go func() {


горутины можно запускать явно через указание go перед вызываемым методом

go sender.startSendMetricsListener(msCtx)

такой подход дает возможность запускать метод как синхронно, так и асинхронно

нет необходимости определять и вызывать анонимные функции в рамках метода

убрал листенер, тк в целом все тормозилось об каналы и в асинхронности нет смысла + обработка ошибок затрудняется - нужен до канал на них

mr-fard4y · 2023-01-30T11:57:20Z

cmd/agent/service/sender/bulk_sender.go

+	sender := &bulkSender{
+		client:         client,
+		batchLimit:     batchLimit,
+		metrics:        make([]*model.Metric, 0, batchLimit),


неиспользуемый параметр
можно убрать из определения

fev0ks · 2023-01-31T06:34:08Z

Огромное спасибо за ревью!
На текущем месте работы такого фидбека очень не хватает :)

mr-fard4y

LGTM

mr-fard4y · 2023-01-31T09:51:07Z

cmd/server/main.go

-	if dbConfig != "" {
-		repository = repositories.NewPgRepository(dbConfig, ctx)
-
+	if appConfig.DBConfig != "" {


27-45 можно инкапсулировать в рамках пакета: вынести в отдельный метод в рамках хранилища/repositories

сейчас реализация копирует "фабричный метод": по параметрам конфига возвращается нужная реализацию хранилища

[Iter-12] Implement batch metric listener

232a3f7

fev0ks force-pushed the origin/feature/sprint_04 branch 5 times, most recently from d5e9ac0 to 3833023 Compare January 23, 2023 18:07

[Iter-14] Add new cpu and mem metrics

26ea990

fev0ks force-pushed the origin/feature/sprint_04 branch 3 times, most recently from 5c53bca to 6675a9a Compare January 23, 2023 19:10

[Iter-14] Split metrics collecting

30ed88c

fev0ks force-pushed the origin/feature/sprint_04 branch from 6675a9a to 30ed88c Compare January 23, 2023 19:17

[Iter-14] Fix races

ee2c6bd

fev0ks force-pushed the origin/feature/sprint_04 branch from 098a6b0 to ee2c6bd Compare January 23, 2023 19:39

[Iter-14] Some fixes

6966ab7

fev0ks force-pushed the origin/feature/sprint_04 branch from 38a3711 to 6966ab7 Compare January 24, 2023 07:00

mipa0717 added 2 commits January 24, 2023 13:15

[Iter-14] please stop 'context deadline exceeded'

f8a2091

[Iter-14] ...

1d1b2a7

mr-fard4y reviewed Jan 28, 2023

View reviewed changes

fev0ks force-pushed the origin/feature/sprint_04 branch 6 times, most recently from 689362d to 4b046f2 Compare January 29, 2023 16:37

[Iter-XXX] Updates after review

f9d0871

fev0ks force-pushed the origin/feature/sprint_04 branch from 4b046f2 to f9d0871 Compare January 29, 2023 16:48

mr-fard4y reviewed Jan 30, 2023

View reviewed changes

Mikhail Patin added 2 commits January 31, 2023 00:07

[Iter-XXX] Updates after review 2

a3348aa

[Iter-XXX] Fix metricsSender abstract type

77e7504

Mikhail Patin and others added 2 commits January 31, 2023 00:52

[Iter-XXX] CollectMetrics: remove redundant ctx

f567e2b

[Iter-XXX] Add constant interval for 'CPUutilization' metric

d8fa5bf

fev0ks force-pushed the origin/feature/sprint_04 branch from edca830 to d8fa5bf Compare January 31, 2023 06:26

mr-fard4y approved these changes Jan 31, 2023

View reviewed changes

fev0ks merged commit 24a407c into main Feb 2, 2023

Sprint 04 #6

Sprint 04 #6

Conversation

fev0ks commented Jan 23, 2023

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

mr-fard4y Jan 28, 2023 • 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

mr-fard4y Jan 28, 2023 • 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

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

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

fev0ks commented Jan 31, 2023

mr-fard4y left a comment

Choose a reason for hiding this comment

mr-fard4y Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

mr-fard4y Jan 28, 2023 •

edited

Loading

mr-fard4y Jan 28, 2023 •

edited

Loading

mr-fard4y Jan 31, 2023 •

edited

Loading