-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Инкремент №15 #15
Инкремент №15 #15
Conversation
ex0rcist
commented
Aug 21, 2024
•
edited
Loading
edited
- Проверку входящей подписи и добавление исходящей подписи реализовал в middleware
- Опросом и отправкой у меня изначально занимались разные горутины, для реализации worker pool я разделил exporter на два вида - batch_exporter остался старый с прошлого спринта, а limited_exporter реализовал отдельными запросами на каждую метрику (как было до batch), кол-во воркеров определяется параметром -l (RATE_LIMIT)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хорошая работа.
exporterKind := detectExporterKind(config) | ||
|
||
switch exporterKind { | ||
case exporter.KindLimited: | ||
exp = exporter.NewLimitedExporter(&config.Address, signer, config.RateLimit) | ||
case exporter.KindBatch: | ||
exp = exporter.NewBatchExporter(&config.Address, signer) | ||
default: | ||
exp, err = nil, fmt.Errorf("unknown exporter type") | ||
} | ||
|
||
return exp, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
detectExporterKind
вызывается только тут, и частично, дублирует логику отсюда, я бы предложил переписать это более компактно:
if c.RateLimit > 0 {
return exporter.NewLimitedExporter(&config.Address, signer, config.RateLimit)
}
return exporter.NewBatchExporter(&config.Address, signer)
Выглядит так, что можно и без ошибки обойтись, так как она нужна только что бы поймать "невалидный" типа экспортера.
if e.err == nil { | ||
return nil | ||
} | ||
|
||
return fmt.Errorf("metrics export failed: %w", e.err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
это кстати можно переписать в одно строку
return fmt.Errorf("metrics export failed: %w", e.err)
если e.err
будет nil
то Errorf
ошибку не вернет
switch value.Kind() { | ||
case metrics.KindCounter: | ||
mex = metrics.NewUpdateCounterMex(name, value.(metrics.Counter)) | ||
|
||
case metrics.KindGauge: | ||
mex = metrics.NewUpdateGaugeMex(name, value.(metrics.Gauge)) | ||
|
||
default: | ||
logging.LogError(entities.ErrMetricReport, "unknown metric") | ||
|
||
e.err = entities.ErrMetricUnknown | ||
return e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
рекомендую заменить на type switch, так будет безопаснее, если кто-то перепутает значения:
swith v := value.(type)
case metrics.Counter:
mex = metrics.NewUpdateCounterMex(name, v)
case metrics.Gauge
mex = metrics.NewUpdateGaugeMex(name, v)
// ....
|
||
url := "http://" + e.baseURL.String() + "/update" | ||
|
||
req, err := http.NewRequest(http.MethodPost, url, payload) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
есть http.NewRequestWithContext, лучше взять за правило всегда использовать запрос с контекстом
return fmt.Errorf("cpu.PercentWithContext: %w", err) | ||
} | ||
|
||
s.CPUutilization = []metrics.Gauge{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Размер []metrics.Gauge{}
можно было указать заранее