fix(metrics, logs): Ensure TelemetryBuffer uses own thread for both size based and periodic flushing - #3084
fix(metrics, logs): Ensure TelemetryBuffer uses own thread for both size based and periodic flushing#3084sl0thentr0py wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
reviewing this with whitespace off recommended
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
8d91cd4 to
0c02d20
Compare
| end | ||
| end | ||
|
|
||
| it "does not conflict with the Sentry logger patch" do |
There was a problem hiding this comment.
moved this here to avoid logger patch leaking into the other test
solnic
left a comment
solnic
left a comment
There was a problem hiding this comment.
I honestly think that at this point we'd be better off using concurrent-ruby's primitives for this stuff, we already depend on that gem anyway.
f1fbf92 to
f875712
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f875712. Configure here.
11050f1 to
0df4484
Compare
sl0thentr0py
changed the title
0df4484 to
5a47e10
Compare
…o that both periodic and size based flushing happen on the buffer thread
5a47e10 to
a34cdba
Compare
|
ran sol pro on this for deadlock analysis |
|
|
||
| it "thread-safely handles concurrent access" do | ||
| expect(client).to receive(:send_envelope).exactly(3).times | ||
| expect(client).to receive(:send_envelope).at_least(:once) |
There was a problem hiding this comment.
@sl0thentr0py I assume that was needed due to jruby, amrite?
There was a problem hiding this comment.
no, not just jruby, it does not always trigger 3 times deterministically now, just whatever wake picks up as pending.
|
|
||
| def wake | ||
| @thread_mutex.synchronize do | ||
| @force_run = true |
There was a problem hiding this comment.
@sl0thentr0py what does this force_run mean? does it cause it to flush immediately without waiting? (just trying to understand the mechanics here).
There was a problem hiding this comment.
yes exactly, it either runs on timeout or when wake_condition triggers, this is an extra boolean that goes together with the wake_condition signal. Can rename to make it clearer, like is_woken.
3 participants
Before this change, the
add_itemtriggered size based flush on the caller thread which is absolutely incorrect behavior.Changes to
ThreadedPeriodicWorker@thread_mutexfor syncing its own internal state@wake_conditionthat is used forwakeing the thread from outside (like size based flushing) - otherwise defaults to theintervalwhich makes it backwards compatible@idle_conditionused for the finalflushthat needs to wait for any running task to finishwakeandwait_for_idleThese are then used in
TelemetryEventBuffer:runandflushare now two different methods,runruns on the buffer thread whereasflushon the main one