Make tracker statistics optional again

[josecelano]

Commit 7abe0f5 introduced an unwanted change. Thread for statistics is always created regardless configuration.

This commit reverts that change. The config option:

config.tracker_usage_statistics

defines wether the statistics should be enabled or not.

[josecelano]

ACK d24af95

[da2ce7]

@josecelano

I find it confusing that calling run_worker() on the stats tracker will enable it, maybe register and enable worker or something would more clearly describe what is happening... What do you think?

Is it possible to write a test to check if the starts are being collected or not?

[josecelano]

@josecelano

I find it confusing that calling run_worker() on the stats tracker will enable it, maybe register and enable worker or something would more clearly describe what is happening... What do you think?

Is it possible to write a test to check if the starts are being collected or not?

Hi @da2ce7 . Yes, I agree. I've already discussed it with @WarmBeer . I did not want to change anything in this PR, just to revert the bug asap.

Regarding testing, the test would depend on how we refactor it. I told @WarmBeer that I see two options:

1. We could have a kind of NullStatsTracker (like /dev/null) that implements the TrackerStatsService trait but does nothing.
2. Or we could make it optional in the Tracker constructor (Option<Box<dyn TrackerStatsService>>).

A more advanced approach could be implementing a middleware or observer pattern. The StatsTracker would be just a new plugin/service. I wanted to open a discussion about it, but I have not done yet.

For the time being, I would merge this PR and create the discussion.

We can rename the method. The method does:

• Create a channel
• Store the channel sender in the current struct so StatsTracker can be used to send events.
• Create a new thread that is the listener.

Maybe we could call it activate_stats_event_listener or something like that.

We could also make small refactors before implementing a more generic approach. I think we could split the class into:

• TrackerStatisticsService (TrackerStatisticsService::bootstrap -> (TrackerStatisticsEventSender, TrackerStatisticsEventListener))
• TrackerStatisticsEventSender
• TrackerStatisticsEventListener
• TrackerStatisticsRepository

But I do not know if it makes sense or if we can go directly to a generic event/listener approach. The events could belong to the Tracker (core) domain, but you can register as many listeners as you want. One of them could be the StatsTracker.

I will elaborate on those ideas in the discussion.

[da2ce7]

@josecelano What about adding a minimal unit test so that we can check that the stats are indeed being/not being collected?

[josecelano]

@josecelano What about adding a minimal unit test so that we can check that the stats are indeed being/not being collected?

OK, I will try. To be honest, I also wanted to do it but, I thought that would be a big effort since we have to deal with concurrency :-). And I was focused on other things. But let's try at least. I think sooner or later we have to do that kind of test. Even if we change the implementation later, what we can learn it's going to be very useful in the future for other tests.

[da2ce7]

Why do we have the same assert:
assert_eq!(stats.tcp4_connections_handled, 0);
assert_eq!(stats.tcp4_connections_handled, 0);
for both the active, and the inactive tests?

[josecelano]

Hey @da2ce7 , It was just a copy/paste typo, but I realized the tests were passing after pushing the commit. I do not know why they pass (why the counter does not work). I'm working on that.

[josecelano]

hey @da2ce7 I've added the test but as you can see the test for the active stats tracker should not pass because the counters are zero. I'm going to continue working on that.

My first idea was an integration or e2e test since the code to enable/disable stats is in the main.rs file. But then, the only way to get stats is by using the API. I can still do it that way, but I decided to move the code to the StatsTracker so that I can add this simpler unit test. The TorrustTracker also uses the configuration in its constructor.

[da2ce7]

Looks good to me :)

[josecelano]

hi @da2ce7 @WarmBeer, I decided to change the testing strategy for this case. I was trying to write a test with the state-based style. I wanted the run the StatsTracker, send a message and assert that the stats change.

The problem with that approach is the worker is an infinite loop. The tests passed if I waited for the worker thread for milliseconds. You can do it this way:

#[tokio::test(flavor = "multi_thread")]
async fn my_test() {
    let stats_tracker = StatsTracker::new_instance(...);

    let result = stats_tracker.send_event(TrackerStatisticsEvent::Tcp4Announce).await;

    // Wait until the worker thread process the event
    println!(...) // A non-blocking wait

    assert!(...);
}

But I did not want to introduce a random wait because:

• It makes tests slower.
• Because it's not deterministic.
• It's a dirty solution :-)

I came up with two ideas:

1. Send an event to abort the loop and wait until the thread finishes.
2. Pass an argument to the run_worker() with the number of events you want to process. After processing those events, the thread finished.

With these two solutions, you can wait until the worker ends processing all the events, and you can get the stats.

I did not like it because we change the behaviour only for testing, and we did not want to refactor just to cover this case with a test.

So, I started to think again about what I was trying to test. I think I could say I was:

"Trying to assert that statistics are collected correctly when they are enabled" and "statistics are not collected when they are disabled".

With the new test version, I only check whether the statistics are collected or not, but I do not test if they are updated correctly. Maybe we could add another test to assert that once the worker receives an event, it updates the statistics correctly. This way, we end up having two unit tests instead of one single test for both things. Of course, we are not testing if the event is sent correctly to the worker. But I suppose the goal for this test was the optional behavior. I mean, testing that we can collect or not the statistics with a configuration option.

Actually, we test that the StatTracker can be enabled/disabled from the constructor, but we are not testing that the main function passes the right value from the configuration. If we want to test that, we have the same "waiting" problem again or we can move the login in the main file to a "bootstrap" module. After the bootstrapping, we can check that StatTracker has the right configuration, which is "empty channel".

[da2ce7]

ACK e12d8e6