wavefrontHQ / wavefront-sdk-go

Wavefront Core Go SDK
Apache License 2.0
4 stars 21 forks source link

chore: refactor background flushing #175

Closed LukeWinikates closed 10 months ago

LukeWinikates commented 10 months ago

This PR separates the responsibility for background flushing of the SDK's buffer channel into a type solely responsible for background flushing.

Prior to this PR, the flush ticker goroutine also applied request throttling logic. This behavior is only ever enabled for the event handler.

The throttling logic used the RealLineHandler's mutex to apply throttling. I'm not sure if that's a good idea or a bad idea, but it relied on running inside the RealLineHandler's ticker goroutine - the Sleep needed to run in a goroutine in order unobtrusively block Flushes until the throttling time had ended.

No existing tests cover this behavior, so we should backfill some before merging this PR.

This PR doesn't use the mutex, but simply remembers the future time when it will be ok to send requests again, and if ~Flush~ FlushWithThrottling is called before that time is up, the FlushWithThrottling uses time.Sleep to wait until the cooldown period ends.

Other Changes in this PR:

idea: is the LineHandlerOption interface worth it given this API is entirely internal anyway?

🟢 the option API allows the NewLineHandler factory to have a small number of required fields ❔compare with standard library types like http.Client, which has a lot of fields, but it seems that all of their zero values are designed to be appropriate defaults, and no other initialization is required.

question: does the throttling logic in flush need to be protected by a mutex or atomic operators?

In practice:

In theory:

idea: allow opt-out of background flushing in the future

The telegraf output plugin has an awkward line where, if the user has set immediate_flush = true it sets the background flush interval to a very large number. Because there is no API for disabling background flushes completely.

LukeWinikates commented 10 months ago
LukeWinikates commented 10 months ago

as of right now, this change does make it so that a 'throttling' LineHandler throttles all flushes, not just the ones that happen in the background.

This is potentially a breaking change, so we could implement a FlushWithThrottle instead.

update: I did add the FlushWithThrottle method.