Skip to content

Conversation

fatalem0
Copy link
Contributor

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Other information

@fatalem0 fatalem0 force-pushed the fatalem0/add-metrics branch 28 times, most recently from 2f5f5a4 to 5ca5531 Compare June 1, 2025 19:12
@fatalem0 fatalem0 marked this pull request as ready for review June 2, 2025 07:06
@fatalem0 fatalem0 force-pushed the fatalem0/add-metrics branch from 5ca5531 to 61557d1 Compare June 2, 2025 13:26
@fatalem0 fatalem0 force-pushed the fatalem0/add-metrics branch 2 times, most recently from b31601c to 3024da9 Compare June 2, 2025 21:24
@rekby
Copy link
Member

rekby commented Jun 3, 2025

Привет, спасибо за PR.

Что понравилось:

  1. Возможность параметризации создаваемой таблицы
  2. Таймауты можно задавать в качестве параметров
  3. Хорошо используются таймауты для завершения измерений
  4. Повторение стиля с rust old/new версиями
  5. учёт тега no slo

Вопросы/пожелания:

  1. Подскажи - почему у тебя в slo-тестах основная программа запускается из папки example.
  2. Почему схема таблицы и схема тестовой строки разнесены в разные крейты, хотя близко связаны по структуре.
  3. Сейчас в статистике считается количество запросов в базу. Для статистики хорошо бы ещё понимать количество ретраев (т.е. когда произошла ретраибельная ошибка и запрос пришлось отправить повторно) и количество неретраибельных ошибок (т.е. когда операция записи/чтения в итоге завершилась с ошибкой).
  4. Для таймаутов и прочих временнЫх интервалос хотелось бы задавать их более человечно, чем в миллисекундах - это сложно читать и понимать, 1s выглядит понятнее, чем 1000 (и из аргументов непонятна единица измерения).
  5. Для типа RateLimiter<NotKeyed, InMemoryState, DefaultClock, NoOpMiddleware> хорошо бы создать псевдоним и использовать его: чтобы было понятно что везде используется один тип и менять его только в одном месте если потребуется другой лимитер. Или другие generic-параметры.
  6. Для таймаутов операций хорошо бы учитывать и таймаут из настроек и отмену контекста. Токен отмены можно даже не передавать как параметр, а использовать при ограничении ожидания самой функции/таски для создания нагрузки.
  7. Параметризация матрицы по версиям SDK сейчас скорее не нужна, т.к. тестируется только нативный SDK. Большого вреда от этого нет, но код немного усложняется.
  8. Зачем использовать metrics.json из slo-action-а? Он не документирован и может в любой момент пропасть или поменяться. Давай тут более автономное решение. Например нагружался может сама сохранять в итоге своей работы файл со статистикой понятного формата, а соседняя утилитка - его анализировать. Утилиту проверки тоже желательно сделать на rust, чтобы работать внутри единой системы типов (читать ровно ту же структуру что сохранилась) и не уходить в программирование на баше - с ним не все хорошо знакомы.
  9. Обратил внимание что ты пользуешься force-merge-ами в git-е. Дальше в этом PR или пожалуйста не ползуйся - тогда мне будет удобно смотреть изменения относительно предыдущей версии.

@rekby rekby added the student good issue for student work. Take from few days to 2 weeks. There is a clear solution. label Jun 3, 2025
@fatalem0 fatalem0 force-pushed the fatalem0/add-metrics branch from 0ef6ef2 to 4d90ae0 Compare June 8, 2025 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
student good issue for student work. Take from few days to 2 weeks. There is a clear solution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants