Skip to content
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

Overhaul stats: start collecting stats per server instance (per socket) #1404

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

josecelano
Copy link
Member

Start collecting stats per server instance (per socket).

Sub-tasks

  • HTTP Tracker Instances.
    • Instantiate independent services (event sender and stat repo) for each instance.
    • Add E2E tests for global metrics (it should fail after the previous refactor).
    • Fix the failing tests (add a listener for the instance to update global metrics).
  • UDP Tracker Instances.

Sorry, something went wrong.

@josecelano josecelano added Enhancement / Feature Request Something New Code Cleanup / Refactoring Tidying and Making Neat - Developer - Torrust Improvement Experience labels Mar 21, 2025
@josecelano josecelano requested a review from da2ce7 March 21, 2025 13:16
@josecelano josecelano self-assigned this Mar 21, 2025
@josecelano
Copy link
Member Author

josecelano commented Mar 21, 2025

This commit creates services per HTTP tracker instances (server bound to a socket address) to collect metrics only for that instance.

There should be test failing because each HTTP tracker instance can use only one event sender and metrics repository. Therefore global metrics should not be updated after this commit.

I think the problem is there are no E2E tests for statistics. There are only integration tests at the HTTP Core package level. That means metrics per instance work.

I will add an E2E tests for global metrics even if we are planning to remove them in the future. In the mean time, it will be covered.

cc @da2ce7

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 0% with 45 lines in your changes missing coverage. Please review.

Project coverage is 81.83%. Comparing base (734393d) to head (a9b1c14).

Files with missing lines Patch % Lines
src/container.rs 0.00% 43 Missing ⚠️
src/console/profiling.rs 0.00% 1 Missing ⚠️
src/main.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1404      +/-   ##
===========================================
- Coverage    82.00%   81.83%   -0.18%     
===========================================
  Files          232      232              
  Lines        17046    17082      +36     
  Branches     17046    17082      +36     
===========================================
  Hits         13979    13979              
- Misses        2831     2867      +36     
  Partials       236      236              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

It creates services per HTTP tracker instance (server bound to a socket address) to collect metrics only for that instace.

There should be test failing becuase any server can use only one event sender and metrics repository.
Therefore global metrics should not be updated.

I think the problem is there are no E2E tests for statistics. There are
only integration tests at the HTTP Core pacakge level.
@josecelano josecelano force-pushed the 1403-overhaul-stats-start-collecting-stats-per-socket-server branch from 42679a1 to a9b1c14 Compare March 21, 2025 13:51
@josecelano
Copy link
Member Author

There is another problem. You can not halt the application with CTRL+C. I think it might be a deadlock. Only the servers before the HTTP servers are started. I'm going to try a different approach. In the AppContainer initialization I initialize all the services needed: the global ones and the duplicates ones that we need for each tracker instance. This way the start function would be pretty much the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- Developer - Torrust Improvement Experience Code Cleanup / Refactoring Tidying and Making Neat Enhancement / Feature Request Something New
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant