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

chore: refactor background flushing #175

Merged
merged 1 commit into from
Nov 17, 2023
Merged

chore: refactor background flushing #175

merged 1 commit into from
Nov 17, 2023

Conversation

LukeWinikates
Copy link
Contributor

@LukeWinikates LukeWinikates commented Oct 24, 2023

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.

  • add RealLineHandler tests using the throttling behavior
  • think about test cases for other behaviors revolving around the mutex

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:

  • shortened the test timeout to 1m from 10m, which should be enough
  • renamed lockOnThrottledError to throttleRequestsOnBackpressure
  • added FlushWithThrottling()
  • made the throttleOnSleepDuration configurable for testing

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:

  • throttling is not used much, because it only applies to events, and events are seldom used
  • background flushing is serial, not parallel, so background flushes will not overlap.
  • user-initiated flushing could overlap with background flushing, which would be a (harmless?) race condition if there is a background request in progress that gets throttled

In theory:

  • it seems as though the background flushing logic may have been intended to allow for concurrent background flushes, and a future change to allow parallel flushing (using a WaitGroup?) might require enabling throttling for all the formats, and might require changes so that the resumeAt field is atomically read and written.

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
Copy link
Contributor Author

LukeWinikates commented Oct 25, 2023

  • think about any edge cases/gotchas about adding the throttling to Flush

@LukeWinikates
Copy link
Contributor Author

LukeWinikates commented Oct 25, 2023

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.

@LukeWinikates LukeWinikates marked this pull request as ready for review October 26, 2023 17:25
internal/flusher.go Outdated Show resolved Hide resolved
@jbooherl jbooherl self-requested a review November 14, 2023 17:13
Copy link
Contributor

@jbooherl jbooherl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LukeWinikates LukeWinikates merged commit d032aee into main Nov 17, 2023
3 checks passed
@LukeWinikates LukeWinikates deleted the ticker-refactor branch November 17, 2023 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants