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

[AbstractLimiter]: inflight: Is this a bug or a feature? #213

Open
ekadet-f9 opened this issue Sep 28, 2024 · 0 comments
Open

[AbstractLimiter]: inflight: Is this a bug or a feature? #213

ekadet-f9 opened this issue Sep 28, 2024 · 0 comments

Comments

@ekadet-f9
Copy link

ekadet-f9 commented Sep 28, 2024

[AbstractLimiter] The inflight count is captured when the listener is acquired (i.e., when a request is accepted), not when the request completes. The onSample() method is passed this 'currentInflight' count.

By the time onSample() is invoked (i.e., after the request completes), the inflight count may have changed significantly because other requests may have started or finished in the interim.

Why It Could Be a Feature:
The current design captures the state of the system (inflight count) at the moment a request is accepted. This gives a snapshot of how "busy" the system was when the request started.
This design could be useful if you're trying to understand the state of the system at the time of accepting requests, not necessarily when they finish.

Why It Could Be a Bug:
If the goal is to measure the load on the system when the request finishes, this implementation could be misleading. The inflight count could differ significantly between the start and end of a request, especially if there are many concurrent requests, leading to inaccurate load measurements.

Suggestion: Fixing the Issue if it’s Considered a Bug

protected Listener createListener() {
    final long startTime = clock.getAsLong();
    inFlight.incrementAndGet();  // Increment inflight count on request start
    
    return new Listener() {
        @Override
        public void onSuccess() {
            int currentInflight = inFlight.decrementAndGet();  // Decrement on success and capture inflight count
            successCounter.increment();
            limitAlgorithm.onSample(startTime, clock.getAsLong() - startTime, currentInflight, false);
        }

        @Override
        public void onIgnore() {
            inFlight.decrementAndGet();  // Decrement on ignore
            ignoredCounter.increment();
        }

        @Override
        public void onDropped() {
            int currentInflight = inFlight.decrementAndGet();  // Decrement on dropped and capture inflight count
            droppedCounter.increment();
            limitAlgorithm.onSample(startTime, clock.getAsLong() - startTime, currentInflight, true);
        }
    };
}

Explanation of Fix:

  • Now the inflight count is captured just before calling onSample(), ensuring that it reflects the inflight count when the request is finished, rather than when it started.
  • This provides a more accurate reflection of the system load at the end of each request.

Conclusion:

  • If you are intentionally logging the inflight count when the request starts (to analyze load at request acceptance), the current behavior is a feature, and no changes are necessary.
  • If you want to reflect the inflight count at the moment the request completes (a more common scenario for load and performance monitoring), then the suggested fix should be applied to ensure accurate measurements.
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

No branches or pull requests

1 participant