Metrics collection #11

jcdkiki · 2025-04-08T17:16:35Z

No description provided.

jcdkiki · 2025-04-08T17:21:23Z

Вот так выглядит нагрузка на все ядра одного воркера ((страшная нагрузка в правой части - это я запустил while true; do echo -n; done в баше в демонстративных целях))

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

Вот так выглядит использование памяти процессом. ((в искусственном примере просто во время "выполнения задачи" происходит malloc на рандомно 0-50 мегабайт))

Время выполнения задачи. Выглядит немного непонятно наверное. этот график показывает "сколько секунд прошло с начала выполнения задачи". поэтому на пиках этого графика будет итоговое время выполнения задачи.

AndrewGavril · 2025-04-10T06:34:26Z

src/metrics-collector.cpp

+    FILE *file = fopen("/proc/stat", "r");
+    char cpu_name[32];
+
+    while (true) {
+        CPUInfo cpu;
+
+        fscanf(file, "%s %lu %lu %lu %lu %*u %*u %*u %*u %*u %*u",
+            cpu_name,
+            &cpu.last_total_user, &cpu.last_total_user_low,
+            &cpu.last_total_sys, &cpu.last_total_idle
+        );
+
+        if (strstr(cpu_name, "cpu") != cpu_name)
+            break;
+
+        cpu.gauge = &cpu_usage_family.Add({{ "cpu", std::string(cpu_name) }});
+        cpu_usage.insert({ std::string(cpu_name), cpu });
+    }
+
+    fclose(file);


Лучше переписать на cpp стиль, не используя функции на Си

AndrewGavril · 2025-04-10T06:34:41Z

src/metrics-collector.cpp

+    FILE *file = fopen("/proc/self/statm", "r");
+    if (file == NULL) {
+        memory_used_gauge->Set(0);
+        return;
+    }
+
+    long mem_pages = 0;
+    fscanf(file, "%ld", &mem_pages);
+    fclose(file);


Аналогично

AndrewGavril · 2025-04-10T06:35:11Z

src/metrics-collector.cpp

+
+void MetricsCollector::getCPUUsage()
+{
+    FILE *file = fopen("/proc/stat", "r");


Аналогично

Ri0n · 2025-04-18T18:59:59Z

src/metrics-collector.cpp

+    thread.join();
+}
+
+void MetricsCollector::getMemoryUsed()


выглядит как функиця двойного назначения 1) прочитать статы 2) обновить метрики.
если утащить чтение статов в безымянный неймспейс в этом cpp, тогда для однострочника записи метрики отделдьная функция не понадобится вовсе и не будет загрязнять неймспейс класса.

Если хочется юнит тесты сделать для коллектора, то такой метод для получения чего-то из /proc сделать как реализацию интерфейсного метода. А в тестах делать инъекцию зависимости с фейковыми данными.

Ri0n · 2025-04-18T19:06:39Z

src/metrics-collector.cpp

+        if (is_task_running) {
+            struct timespec time;
+            clock_gettime(CLOCK_MONOTONIC, &time);
+            task_processing_time_gauge->Set((time.tv_sec - task_start.tv_sec) + (time.tv_nsec - task_start.tv_nsec) * 1e-9);


ещё одно место где стоит подумать о C++ и в частности о std::chrono

Ri0n · 2025-04-18T19:08:39Z

src/metrics-collector.cpp

+{
+    is_task_running = false;
+    task_processing_time_gauge->Set(0);
+    gateway.PushAdd();


обилие разного кодестайла...

Ri0n · 2025-04-18T19:24:39Z

src/metrics-collector.hpp

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


здесь нужен атомик и может даже вообще передавать его из вне чтоб можно было из посикс сигналов ставить (по sigint например).

кажется при конструировании у нас не было никакой возможности получить здесь значение отличное от true. Видимо можно было сразу здесь и прописать
std::atomic<bool> is_running{true};

Ri0n · 2025-04-18T19:26:23Z

src/metrics-collector.hpp

+
+    struct CPUInfo {
+        prometheus::Gauge *gauge;
+        uint64_t last_total_user;


а что там за гигантские значения хранятся что uint64 понадобился?

а что там за гигантские значения хранятся что uint64 понадобился?

Чтобы переполнения реже происходили. Это время, как в выводе time. А процент использования вычисляется уже как (user+sys) / total

Ri0n · 2025-04-18T19:42:54Z

src/hash.cpp

+    }
+  std::cout << std::endl;
+
+  EVP_cleanup();


код писался под какую-то древнюю версию openssl?
в любом случае это выглядит плохим решением что-то глобально клинить когда могут быть ещё таски

А это не мой код, делал чекаут от #4. Там уже всё переписано и замерджено, подолью мейн сюда и не будет этого

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

Ri0n · 2025-04-18T19:46:42Z

src/hash.cpp

+  return 0;
+}
+
+int main_metrics(int argc, char* argv[]) {


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

Всё это исчезнет и останется один единственный main, когда сюда прикрутится полноценный воркер (#15)

Ri0n · 2025-04-18T19:49:00Z

src/hash.cpp

+    return 1;
+  }
+
+  MetricsCollector metrics_collector(argv[2], argv[3], argv[4]);


предлагаю для опций использовать это https://www.boost.org/doc/libs/1_88_0/doc/html/program_options/tutorial.html#id-1.3.29.4.3

Устанавливать целый буст ради одной фичи?
Может лучше передавать все аргументы через env переменные? Удобно будет при запуске докер контейнера их передавать

Ri0n · 2025-04-18T19:55:20Z

src/hash.cpp

+
+  MetricsCollector metrics_collector(argv[2], argv[3], argv[4]);
+
+  srand(time(NULL));


не настаиваю но
https://cplusplus.com/reference/random/mersenne_twister_engine/mersenne_twister_engine/
:)

Это тоже исчезнет, когда здесь будет полноценный воркер, а не sleep(random).

Ri0n · 2025-04-18T20:04:14Z

src/hash.cpp

+
+    // some "work" that requires memory
+    void *data = malloc(rand() % (1024 * 1024 * 50)); // 0 - 50 megabytes
+    std::this_thread::sleep_for(std::chrono::seconds(rand() % 5 + 1));


sleep_for не помечен как noexcept, значит это место потециальной утечки

AndrewGavril

Нужно исправить конфликт

Ri0n · 2025-04-22T19:10:33Z

src/main.cpp

+}
+
+int metrics_main(int argc, char *argv[]) {
+    signal(SIGTERM, signal_handler);


sigaction ?

Ri0n · 2025-04-22T19:11:49Z

src/main.cpp

+}
+
+
+bool is_running = true;


а чего их теперь два?

надо второй сделать референсом первого а здесь добавить атомик

Ri0n · 2025-04-22T19:13:27Z

src/main.cpp

+#include <thread>
+#include <signal.h>
+
+void print_usage(const std::string& program_name) {


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

Ri0n · 2025-04-22T19:23:08Z

src/main.cpp

+
+int main(int argc, char *argv[])
+{
+    static const char usage[] = "Specify hash/metrics";


fyi https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rio-endl

Ri0n · 2025-04-22T19:26:42Z

src/main.cpp

+        return 1; 
+    }
+
+    if (strcmp(argv[1], "hash") == 0) {


У всех фломастеры разные конечно, но я бы наверное сделал так

std::string_view cmd{argv[1]}; if (cmd == "hash") { ...

или даже

cmd == "hash"sv

Ri0n · 2025-04-22T20:07:22Z

src/md_calculator.cpp

+MDCalculator::MDCalculator(const std::string& algorithm) 
+    : md_(nullptr), ctx_(nullptr, EVP_MD_CTX_free) {
+
+    OpenSSL_add_all_digests();


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

Ri0n · 2025-04-22T20:17:49Z

src/metrics-collector.cpp

+        if (total_user < cpu.last_total_user || total_user_low < cpu.last_total_user_low ||
+            total_sys < cpu.last_total_sys || total_idle < cpu.last_total_idle) {
+            // overflow detection
+            percent = -1.0;


Я сделал небольшие подсчеты для своего железа. Для этого понадобится 1+ млрд лет. Думаю система отключится раньше.
Но респект что переполнение вообще пришло в голову.
А вот с точки зрения пользователя -1 это нонсенс.

add metrics collector class

b019842

jcdkiki linked an issue Apr 8, 2025 that may be closed by this pull request

Develop class for metrics collection in worker node #7

Open

AndrewGavril requested changes Apr 10, 2025

View reviewed changes

use std::ifstream instead of FILE*

60d8ead

jcdkiki requested a review from AndrewGavril April 15, 2025 19:57

Ri0n reviewed Apr 18, 2025

View reviewed changes

jcdkiki requested a review from Ri0n April 20, 2025 15:16

AndrewGavril changed the base branch from main to calculate_hash April 20, 2025 19:18

AndrewGavril changed the base branch from calculate_hash to main April 20, 2025 19:18

jcdkiki force-pushed the metrics-collection branch from 92170c1 to ea2d3e2 Compare April 20, 2025 19:47

jcdkiki added 2 commits April 20, 2025 23:09

merge main

3695e63

metrics-collector fixes

0329b73

jcdkiki force-pushed the metrics-collection branch from ea2d3e2 to 0329b73 Compare April 20, 2025 20:20

AndrewGavril requested changes Apr 22, 2025

View reviewed changes

Ri0n reviewed Apr 22, 2025

View reviewed changes

jcdkiki added 2 commits May 5, 2025 02:52

Merge branch 'main' into metrics-collection

35e1607

make is_running atomic

c0b67bc

jcdkiki requested a review from AndrewGavril May 5, 2025 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metrics collection #11

Metrics collection #11

jcdkiki commented Apr 8, 2025

jcdkiki commented Apr 8, 2025 •

edited

Loading

AndrewGavril Apr 10, 2025

AndrewGavril Apr 10, 2025

AndrewGavril Apr 10, 2025

Ri0n Apr 18, 2025

Ri0n Apr 18, 2025

Ri0n Apr 18, 2025

Ri0n Apr 18, 2025

Ri0n Apr 18, 2025

jcdkiki Apr 20, 2025

Ri0n Apr 18, 2025

jcdkiki Apr 20, 2025

Ri0n Apr 22, 2025

Ri0n Apr 18, 2025

jcdkiki Apr 20, 2025

Ri0n Apr 18, 2025

jcdkiki Apr 20, 2025

Ri0n Apr 18, 2025

jcdkiki Apr 20, 2025

Ri0n Apr 18, 2025

AndrewGavril left a comment

Ri0n Apr 22, 2025

Ri0n Apr 22, 2025

Ri0n Apr 22, 2025

Ri0n Apr 22, 2025

Ri0n Apr 22, 2025

Ri0n Apr 22, 2025

Ri0n Apr 22, 2025


		MetricsCollector metrics_collector(argv[2], argv[3], argv[4]);

		srand(time(NULL));

Metrics collection #11

Are you sure you want to change the base?

Metrics collection #11

Conversation

jcdkiki commented Apr 8, 2025

jcdkiki commented Apr 8, 2025 • 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

AndrewGavril 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

jcdkiki commented Apr 8, 2025 •

edited

Loading