feat: add metrics by vovanbravin · Pull Request #163 · moevm/grpc_server

vovanbravin · 2026-05-14T18:15:55Z

No description provided.

AndrewGavril

Не годится. Метрики controller получать не должен. Все метрики от воркеров пишутся сразу в prometheus->grafana. У контроллера могут быть свои метрики, например, длительность ответа на получение профиля фильтра, количество таких запросов в каждый момент времени и т.д. А метрики фильтрации -- это метрики воркеров.

AndrewGavril · 2026-05-22T14:14:55Z

 #ifndef WORKER_HPP
 #define WORKER_HPP

+#include "../include/metrics_collector.hpp"


Относительный путь нужно убрать

Не исправлено

KonstantinKondratenko

Я немного не понял, что такое FiltrWorker и почему он не в diff -- если это какое-то легаси, то нужно убрать, если это ваши наработки относительно старые, то тоже по идее надо убрать, т.к. там логика какая-то странная
http://github.com/moevm/grpc_server/blob/metrics/worker/src/main.cpp

Если я правильно понял, тут как раз легаси, т.к. этот https://github.com/moevm/grpc_server/blob/2a42dc5ad7187879b4028ac8f9a73ba833324271/worker/src/main.cpp

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

вот этот старый непонятный я бы вырезал, чтобы он дэдкодом не висел
в файле много коммитов было, там изменен нейминг с HashWorker на FiltrWorker
но что там за MetricsCollector metrics_collector;
и что за логика метрик -- не понятно.

Если не используется, давайте вырезать

KonstantinKondratenko · 2026-05-24T20:31:10Z

+  MetricsCollector(const char *gateway_address, const char *gateway_port,
+                   const char *worker_name);


а вы это где-то используете?

KonstantinKondratenko · 2026-05-24T21:02:19Z


+void Worker::RecordPacketReceived() {
+  packets_received_count++;
+  if (metrics_collector_) {


а где он создается?

KonstantinKondratenko

Я очень за двуслойную архитектуру вместо текущей
проблема текущей:

получили пакет
быстро обраболтали
начали записывать метрику
- полезли в прометеус, какие-то лейблы там, работа с БД (под капотом но все же)
пошли дальше

Нужно перейти на 2 слоя:

Cлой 1

получили пакет
обработали пакет
увеличили локальный счетчик
пошли дальше

Слой 2

раз в X секунд (1-15-30-60) -- как выберем отправляем накопившиеся данные
обнуляет локальный счетчик

Это будет быстро и не будет батлнека в работе с прометеусом

KonstantinKondratenko · 2026-05-24T21:19:35Z

+  if (packets_dropped_by_reason_family) {
+    packets_dropped_by_reason_family->Add({{"reason", reason}})
+        .Increment(count);


Важный вопрос оптимизации
Каждый раз дергать прометеуса плохо в рамках концепции быстрого фильтра -- нужно переработать логику

KonstantinKondratenko · 2026-05-24T21:20:25Z

+void MetricsCollector::IncrementBlockedDomain(const std::string &domain_or_ip) {
+  if (blocked_domains_family) {
+    blocked_domains_family->Add({{"domain", domain_or_ip}}).Increment();
+  }
+}
+


вот это вообще опасным кажется
представьте у вас 8000 доменов в базе прометеуса и и на каждый заблокированный пакет будет перебор базы -- у нас фильтр будтем медленный

KonstantinKondratenko · 2026-06-05T13:36:33Z

почему PR в смерженную ветку а не в нашу основную?
#156

Исрпавьте, подтяните измнения

vovanbravin · 2026-06-10T11:10:46Z

@KonstantinKondratenko изменил на базовую ветку

AndrewGavril · 2026-06-10T11:32:18Z

 #ifndef WORKER_HPP
 #define WORKER_HPP

+#include "../include/metrics_collector.hpp"


Не исправлено

KonstantinKondratenko

Вы тестируете своей код?
@vovanbravin

По логике, все, что я описал в коментариях приведет к тому, что

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

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

KonstantinKondratenko · 2026-06-15T14:12:32Z

    build: 
      context: .
      dockerfile: Dockerfile.cc_x86_to_x86
+    environment:


Вы тестировали код?
у вам в env переменных нет METRICS_GATEWAY_ADDRESS, а в worker\src\main.cpp :

const char *gateway_address = getenv("METRICS_GATEWAY_ADDRESS"); const char *gateway_port = getenv("METRICS_GATEWAY_PORT"); if (gateway_address == nullptr || gateway_port == nullptr) { spdlog::error("Environment variables are not fully specified. " "Specify METRICS_GATEWAY_ADDRESS and METRICS_GATEWAY_PORT"); return 1; }

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

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

admin/README.md | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----- controller/internal/service/client/http_client.go | 6 +++---

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

Ну вот хорошо, вы PR подготваливаете разве так, чтобы он работал только на тестовом стенде?
Нужно согласовывать переменные, чтобы не ломать сборку

Поправлю

KonstantinKondratenko · 2026-06-15T14:16:07Z

                              struct net_port *port_out,
                              uint16_t queue_number) {
  if (solution_is_send) {
+    record_packet_passed();


Перенесите в область после rte_eth_tx_burst -- иначе, если не отправится, то расхождение статистики будет
плюс еще важно было бы проверять, сколько пакетов де-факто отправилось, если я доку правильно понял, то функция как раз это и возвращает -> полезно это куда-то принимать и как-то анализировать
https://doc.dpdk.org/api/rte__ethdev_8h.html#a83e56cabbd31637efd648e3fc010392b

KonstantinKondratenko · 2026-06-15T14:29:30Z

 #ifndef WORKER_HPP
 #define WORKER_HPP

+#include "metrics_collector.hpp"


линтер ругается поправьте

KonstantinKondratenko · 2026-06-15T14:39:14Z


  for (int i = 0; i < nb_rx; i++) {

+    record_packet_received();


вы тут считает пакет, а дальше идут логические ветки, которые ловят ошибки, например, Failed to search a key-value pair in the hash table -- а у вас такой пакет уже посчитан и пакет не считается ни passed, ни dropped
из-за этого ломается концепция метрик, что мы считаем:
received != passed + dropped -- нужно везде добавить такую обработку, чтобы расхождения не было, например добавлять
record_packet_droped(); и rte_pktmbuf_free(pkts[i]);

KonstantinKondratenko · 2026-06-15T16:02:48Z


 bool Worker::classify(const std::string &type, const std::string &target,
                      struct requested_classification *out_req) {
+  RecordTaskStart();


у вас в classify вызываются RecordTaskStart и RecordTaskEnd, которые принудительно пушат метрики, а если я правильно понимаю, то классифкация запускается при миссе кэша.

что получаем -- что каждый мисс кэша генерирует 2 синхронные операции, что ломает концепт двух слоев -- горячего (пайплайн обработки) и холодного (фоновая выгрузка метрик)

я бы убрал принудительный синхронный пуш из RecordTaskStart и RecordTaskEnd

KonstantinKondratenko · 2026-06-15T16:10:23Z

+  void IncrementPacketsReceived(int count = 1);
+  void IncrementPacketsPassed(int count = 1);
+  void IncrementPacketsDropped(const std::string &reason, int count = 1);
+  void IncrementBlockedDomain(const std::string &domain_or_ip);


где реализация?
Если забыли -- допишите
Если нет ее (почему?) -- то уберите объявление

KonstantinKondratenko · 2026-06-15T16:11:01Z


-  void GetCPUUsage();
-  void MainLoop();
+  prometheus::Histogram *metrics_collection_duration;


поле объявлено и не используется

KonstantinKondratenko · 2026-06-15T16:12:08Z

+  PushMetrics();
 }
+
+void MetricsCollector::ObserveCollectionDuration(double seconds) {


какой совместимости?
Выглядит как велосипед какой-то...

Если нам он не нужен, то уберите, или реализуйте логику, в которой он используется...

KonstantinKondratenko · 2026-06-15T16:12:52Z

+  }
+}
+
+void MetricsCollector::IncrementPacketsDropped(const std::string &reason,


reason не используется нигде и никак -- почему?
так не должно быть, у нас конракты какие-то ненадежные

KonstantinKondratenko · 2026-06-15T16:27:28Z

  }
 }

 void Worker::statsReport() {


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

у вас в controller\pkg\proto\communication\communication.proto контракт:
message StatsReport {
uint64 worker_id = 1;
uint64 time = 2;
uint64 total_blocked = 3;
uint64 total_allowed = 4;
repeated ResourceStats resources = 5;
}

вы заполняете
report.set_worker_id(worker_id);
report.set_time(time(nullptr));

т.е. функция делает примерно следующее "я воркер, вот время" -- итого:

полезной нагрузки 0

эксплуатирует старую концепцию взаимодействия по метрикам воркера и контроллера

в Worker::MainLoop вы дергаете этот метод, а зачем???

Зачем оно все еще в коде и все еще вызывается??????

KonstantinKondratenko · 2026-06-19T14:15:37Z

Линтер поправьте

KonstantinKondratenko · 2026-06-26T11:56:49Z

Не исправлена IP ветка обработки пакета -- аналогично #163 (comment)

KonstantinKondratenko

поправьте это + комментарии (которые не в рамках ревью, а чуть выше)

KonstantinKondratenko · 2026-06-26T11:58:14Z

+  uint64_t dropped = local_packets_dropped.exchange(0);
+
+  if (received > 0)
+    metrics_collector_->IncrementPacketsReceived(received);


передаете тип uint64_t , а принимаете int в metrics_collector.hpp и metrics_collector.cpp

сделайте везде uint64_t

KonstantinKondratenko · 2026-06-26T12:01:33Z


+  std::unordered_map<std::string, CPUInfo> cpu_usage;
  prometheus::Gauge *memory_used_gauge;
  prometheus::Gauge *task_processing_time_gauge;


вы с этими полями не работаете, как я понимаю -- это легаси какое-то, нужно убрать
tasks_completed_counter
tasks_completed_counter
std::atomic is_task_running{false};
std::chrono::time_pointstd::chrono::high_resolution_clock task_start;

KonstantinKondratenko · 2026-06-26T12:02:49Z

+  std::atomic<uint64_t> local_packets_dropped{0};
+
+  std::chrono::steady_clock::time_point last_metrics_push_time;
+  const int METRICS_PUSH_INTERVAL_SEC = 5;


мы же отошли от кривого нейминга, когда у нас push назван, а идейно это не push

KonstantinKondratenko · 2026-06-26T12:04:15Z

+
+extern void record_packet_received();
+extern void record_packet_passed();
+extern void record_packet_droped();


record_packet_droped -> record_packet_dropped

vovanbravin · 2026-07-02T13:51:23Z

@KonstantinKondratenko проверьте поправки

vovanbravin added 2 commits May 14, 2026 21:14

feat: add metrics

767cc95

fix linter

f155a86

vovanbravin linked an issue May 15, 2026 that may be closed by this pull request

Метрики worker #146

Closed

KonstantinKondratenko requested a review from AndrewGavril May 15, 2026 14:42

KonstantinKondratenko added the need_review label May 15, 2026

AndrewGavril requested changes May 15, 2026

View reviewed changes

KonstantinKondratenko removed the need_review label May 15, 2026

fix push

8672009

KonstantinKondratenko requested a review from AndrewGavril May 21, 2026 13:08

fix build

fd6914e

vovanbravin added invalid This doesn't seem right in progress and removed invalid This doesn't seem right labels May 22, 2026

AndrewGavril requested changes May 24, 2026

View reviewed changes

KonstantinKondratenko requested changes May 24, 2026

View reviewed changes

KonstantinKondratenko reviewed May 24, 2026

View reviewed changes

KonstantinKondratenko requested changes May 24, 2026

View reviewed changes

vovanbravin added 4 commits May 27, 2026 19:26

add local count

0107726

Merge traffic_filtering into metrics

c2a3b3d

fix: count packets when filter:off

052c32a

fix linter

218a5c7

vovanbravin changed the base branch from add_ip_in_worker to traffic_filtering June 10, 2026 11:09

AndrewGavril requested changes Jun 10, 2026

View reviewed changes

fix: path and fprintf

74e6658

AndrewGavril approved these changes Jun 11, 2026

View reviewed changes

AndrewGavril requested a review from KonstantinKondratenko June 11, 2026 16:14

KonstantinKondratenko requested changes Jun 15, 2026

View reviewed changes

fix

397a0cd

fix build

aaab892

KonstantinKondratenko requested changes Jun 26, 2026

View reviewed changes

vovanbravin added 2 commits July 2, 2026 16:09

fix

896ad1e

fix record_packets_dropped

98ebcd9

vovanbravin added need_review and removed in progress labels Jul 3, 2026

KonstantinKondratenko merged commit d3ef929 into traffic_filtering Jul 6, 2026
5 of 6 checks passed

		MetricsCollector(const char gateway_address, const char gateway_port,
		const char *worker_name);

Uh oh!

Conversation

vovanbravin commented May 14, 2026

Uh oh!

AndrewGavril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KonstantinKondratenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KonstantinKondratenko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Cлой 1

Слой 2

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KonstantinKondratenko commented Jun 5, 2026

Uh oh!

vovanbravin commented Jun 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KonstantinKondratenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KonstantinKondratenko commented Jun 19, 2026

Uh oh!

KonstantinKondratenko commented Jun 26, 2026

Uh oh!

KonstantinKondratenko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

KonstantinKondratenko left a comment •

edited

Loading

KonstantinKondratenko left a comment •

edited

Loading