NH-124861: restart timer thread

[xuan-cao-swi]

Description

This is a hot fix for 7.0.1 that restart the timer thread after fork.
Also add the thread lock that synchronize the token bucket variable changes.
Depreciated the config option :sample_rate (seems no longer used)

Test (if applicable)

[cheempz]

The token bucket replenishment approach is different from JS/Python--it seems a timer thread on interval adds the token here, while the other two implementations "replenish during consume", meaning, at the time a token consume call is made, the interval between last consume and now is calculated, then the number of tokens that accrued during the interval is added.

Was there a reason you went with this different approach?

[cheempz]

Minor point is that given this difference, and if you intend to keep this different implementation, it does not make sense to set a MAX_INTERVAL--there should just be a DEFAULT_INTERVAL that is 1 second. The intention with JS/Python MAX_INTERVAL is to guard against the scenario where no consume is called for a long time, i.e. app sits there w/o receiving request.

[xuan-cao-swi]

I think apm-ruby approach is similar to apm-js.

apm-js seems also has this timer that run task, which add token with duration of interval. Although it's not a thread (e.g. all single thread in js), but I think setInterval function from js create an endless loop to keep executing the task function.

I checked the apm-python, it indeed recalculates during each consume and update.

[cheempz]

Ah my bad, i just assumed the Python implementation is based on JS.

Question for @tammy-baylis-swi or @jerrytfleung now... do you remember the reason for Python implementation to not use a background thread to replenish tokens?

And @xuan-cao-swi how does the "reinit thread for forked child process" work in Ruby--not seeing anything obvious in this PR that corresponds to the Python register_at_fork.

[xuan-cao-swi]

not seeing anything obvious in this PR

Right, there is no reinitialize the thread like reset_on_fork from http_sampler.
The update/update_from_hash will start the @timer thread after fork. Because after fork, the settings_request will start, and it will update the settings; it will restart the timer thread in line with start unless running.

[cheempz]

Re:

reason for Python implementation to not use a background thread to replenish tokens?

wondering if due to GIL where the thread might not actually run as consistently on interval as we'd like in a CPU bound application, thus not replenishing tokens accurately.

[cheempz]

The logic is hard to follow due to many threads and methods involved, but IIUC the settings are being updated in a single background thread that runs the update_settings logic, which uses a mutex to protect the @settings data. But not seeing this mutex used to synchronize the reading of @settings which seems to happen in get_settings. Seems we're missing the case of using the settings mutex to synchronize reads, e.g. multiple application threads could be calling shouldSample (thus getting settings), while this settings update thread is trying to write it?

Then as part of update_settings it calls this update_from_hash method, and we're using another mutex here to protect concurrent access of the @rate, @capacity and @tokens variables during token consume/replenish, right?

I'm also still confused why we could be getting settings[:interval] and needing to update it here.

Finally, i think the APM Python way is easier on the brain. One less thread to think about.

[xuan-cao-swi]

Seems we're missing the case of using the settings mutex to synchronize reads

Yes, I will add them later

I'm also still confused why we could be getting settings[:interval] and needing to update it here.

Yeah, the interval part should be removed

APM Python way is easier on the brain.

Agree, especially with the thread, the mutex will make everything complex.

[cheempz]

Thanks @xuan-cao-swi. As discussed we can get this in, then actually revamp the thread safety logic. Minor is we should also get the sample_rate deprecation into main if not already there.